[PATCH/RFC v2 net-next 0/4] ravb: Add support for r8a7795 SoC

[PATCH/RFC v2 net-next 0/4] ravb: Add support for r8a7795 SoC

[Simon Horman]

This series enhances the ravb driver to support the r8a7795 SoC.

Changes:

v1 -> v2
* Address extensive review from Sergei Shtylyov and  Geert Uytterhoeven
  - In particular mandate all 25 interrupts in binding
* Broke brinding change into a separate patch
* New 100Mbit/s limit implementation in ravb driver using
  proposed update to phylib
* Split integration and driver changes into separate patch-sets

Base:

This is based on net-next/master

Availability:

* I will separately post a series,
  "[PATCH/RFC v2 0/5] arm64: dts: r8a7795: enable EthernetAVB on Salvator-X",
  to the linux-sh list to integrate the ravb on the r8a7795/salvatore-x board
* To aid review I will push the following to the renesas tree:
  - driver (this series): (me/r8a7795-ravb-driver-v2)
  - integration: me/r8a7795-ravb-integration-v2
  - both with runtime dependencies: me/r8a7795-ravb-driver-and-integration-v2.runtime
  
Kazuya Mizuguchi (3):
  ravb: Provide dev parameter to DMA API
  ravb: Document binding for r8a7795 SoC
  ravb: Add support for r8a7795 SoC

Simon Horman (1):
  phylib: Add phy_set_max_speed helper

 .../devicetree/bindings/net/renesas,ravb.txt       | 65 +++++++++++++--
 drivers/net/ethernet/renesas/ravb.h                |  7 ++
 drivers/net/ethernet/renesas/ravb_main.c           | 95 +++++++++++++++-------
 drivers/net/phy/phy_device.c                       | 52 +++++++-----
 include/linux/phy.h                                |  1 +
 5 files changed, 166 insertions(+), 54 deletions(-)

-- 
2.1.4

[PATCH/RFC v2 net-next 1/4] phylib: Add phy_set_max_speed helper

[Simon Horman]

Add a helper to allow ethernet drivers to limit the speed of a phy
(that they are attached to).

This mainly involves factoring out the business-end of
of_set_phy_supported() and exporting a new symbol.

This code seems to be open coded in several places, in several different
variants.

This code is envisaged this will be used in situations where setting
the "max-speed" property is not appropriate, e.g. because the maximum
speed is not a property of the phy hardware.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---

v2
* First post
---
 drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++----------------
 include/linux/phy.h          |  1 +
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c0f211127274..d9a020095972 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
+{
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return;
+
+	/* The default values for phydev->supported are provided by the PHY
+	 * driver "features" member, we want to reset to sane defaults fist
+	 * before supporting higher speeds.
+	 */
+	phydev->supported &= PHY_DEFAULT_FEATURES;
+
+	switch (max_speed) {
+	default:
+		return;
+
+	case SPEED_1000:
+		phydev->supported |= PHY_1000BT_FEATURES;
+	case SPEED_100:
+		phydev->supported |= PHY_100BT_FEATURES;
+	case SPEED_10:
+		phydev->supported |= PHY_10BT_FEATURES;
+	}
+}
+
+void phy_set_max_speed(struct phy_device *phydev, u32 max_speed)
+{
+	__set_phy_supported(phydev, max_speed);
+	phydev->advertising = phydev->supported;
+}
+EXPORT_SYMBOL(phy_set_max_speed);
+
 static void of_set_phy_supported(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->dev.of_node;
@@ -1216,25 +1247,8 @@ static void of_set_phy_supported(struct phy_device *phydev)
 	if (!node)
 		return;
 
-	if (!of_property_read_u32(node, "max-speed", &max_speed)) {
-		/* The default values for phydev->supported are provided by the PHY
-		 * driver "features" member, we want to reset to sane defaults fist
-		 * before supporting higher speeds.
-		 */
-		phydev->supported &= PHY_DEFAULT_FEATURES;
-
-		switch (max_speed) {
-		default:
-			return;
-
-		case SPEED_1000:
-			phydev->supported |= PHY_1000BT_FEATURES;
-		case SPEED_100:
-			phydev->supported |= PHY_100BT_FEATURES;
-		case SPEED_10:
-			phydev->supported |= PHY_10BT_FEATURES;
-		}
-	}
+	if (!of_property_read_u32(node, "max-speed", &max_speed))
+		__set_phy_supported(phydev, max_speed);
 }
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 962387a192f1..692b202a52af 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -794,6 +794,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
 int phy_start_interrupts(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
 void phy_device_free(struct phy_device *phydev);
+void phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 
 int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask,
 		       int (*run)(struct phy_device *));
-- 
2.1.4

[PATCH/RFC v2 net-next 2/4] ravb: Provide dev parameter to DMA API

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch is in preparation for using this driver on arm64 where the
implementation of __dma_alloc_coherent fails if a device parameter is not
provided.

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Masaru Nagai <masaru.nagai.vx@renesas.com>
[horms: squashed into a single patch]
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---
* [horms]
  I have only tested this on arm64.
  It should be tested for regressions on arm hardware.

v0 [Kazuya Mizuguchi, Yoshihiro Shimoda, Masaru Nagai]

v1 [Simon Horman]
* Squashed into a single patch

v2 [Simon Horman]
* No change
---
 drivers/net/ethernet/renesas/ravb_main.c | 38 ++++++++++++++++----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 450899e9cea2..4ca093d033f8 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -201,7 +201,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	if (priv->rx_ring[q]) {
 		ring_size = sizeof(struct ravb_ex_rx_desc) *
 			    (priv->num_rx_ring[q] + 1);
-		dma_free_coherent(NULL, ring_size, priv->rx_ring[q],
+		dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
 				  priv->rx_desc_dma[q]);
 		priv->rx_ring[q] = NULL;
 	}
@@ -209,7 +209,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	if (priv->tx_ring[q]) {
 		ring_size = sizeof(struct ravb_tx_desc) *
 			    (priv->num_tx_ring[q] * NUM_TX_DESC + 1);
-		dma_free_coherent(NULL, ring_size, priv->tx_ring[q],
+		dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q],
 				  priv->tx_desc_dma[q]);
 		priv->tx_ring[q] = NULL;
 	}
@@ -240,13 +240,13 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 		rx_desc = &priv->rx_ring[q][i];
 		/* The size of the buffer should be on 16-byte boundary. */
 		rx_desc->ds_cc = cpu_to_le16(ALIGN(PKT_BUF_SZ, 16));
-		dma_addr = dma_map_single(&ndev->dev, priv->rx_skb[q][i]->data,
+		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  ALIGN(PKT_BUF_SZ, 16),
 					  DMA_FROM_DEVICE);
 		/* We just set the data size to 0 for a failed mapping which
 		 * should prevent DMA from happening...
 		 */
-		if (dma_mapping_error(&ndev->dev, dma_addr))
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
 			rx_desc->ds_cc = cpu_to_le16(0);
 		rx_desc->dptr = cpu_to_le32(dma_addr);
 		rx_desc->die_dt = DT_FEMPTY;
@@ -309,7 +309,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 
 	/* Allocate all RX descriptors. */
 	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
-	priv->rx_ring[q] = dma_alloc_coherent(NULL, ring_size,
+	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
 					      &priv->rx_desc_dma[q],
 					      GFP_KERNEL);
 	if (!priv->rx_ring[q])
@@ -320,7 +320,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	/* Allocate all TX descriptors. */
 	ring_size = sizeof(struct ravb_tx_desc) *
 		    (priv->num_tx_ring[q] * NUM_TX_DESC + 1);
-	priv->tx_ring[q] = dma_alloc_coherent(NULL, ring_size,
+	priv->tx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
 					      &priv->tx_desc_dma[q],
 					      GFP_KERNEL);
 	if (!priv->tx_ring[q])
@@ -443,7 +443,7 @@ static int ravb_tx_free(struct net_device *ndev, int q)
 		size = le16_to_cpu(desc->ds_tagl) & TX_DS;
 		/* Free the original skb. */
 		if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
-			dma_unmap_single(&ndev->dev, le32_to_cpu(desc->dptr),
+			dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
 					 size, DMA_TO_DEVICE);
 			/* Last packet descriptor? */
 			if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
@@ -546,7 +546,7 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q)
 
 			skb = priv->rx_skb[q][entry];
 			priv->rx_skb[q][entry] = NULL;
-			dma_unmap_single(&ndev->dev, le32_to_cpu(desc->dptr),
+			dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
 					 ALIGN(PKT_BUF_SZ, 16),
 					 DMA_FROM_DEVICE);
 			get_ts &= (q == RAVB_NC) ?
@@ -586,14 +586,14 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q)
 			if (!skb)
 				break;	/* Better luck next round. */
 			ravb_set_buffer_align(skb);
-			dma_addr = dma_map_single(&ndev->dev, skb->data,
+			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
 						  le16_to_cpu(desc->ds_cc),
 						  DMA_FROM_DEVICE);
 			skb_checksum_none_assert(skb);
 			/* We just set the data size to 0 for a failed mapping
 			 * which should prevent DMA  from happening...
 			 */
-			if (dma_mapping_error(&ndev->dev, dma_addr))
+			if (dma_mapping_error(ndev->dev.parent, dma_addr))
 				desc->ds_cc = cpu_to_le16(0);
 			desc->dptr = cpu_to_le32(dma_addr);
 			priv->rx_skb[q][entry] = skb;
@@ -1300,8 +1300,8 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		 entry / NUM_TX_DESC * DPTR_ALIGN;
 	len = PTR_ALIGN(skb->data, DPTR_ALIGN) - skb->data;
 	memcpy(buffer, skb->data, len);
-	dma_addr = dma_map_single(&ndev->dev, buffer, len, DMA_TO_DEVICE);
-	if (dma_mapping_error(&ndev->dev, dma_addr))
+	dma_addr = dma_map_single(ndev->dev.parent, buffer, len, DMA_TO_DEVICE);
+	if (dma_mapping_error(ndev->dev.parent, dma_addr))
 		goto drop;
 
 	desc = &priv->tx_ring[q][entry];
@@ -1310,8 +1310,8 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	buffer = skb->data + len;
 	len = skb->len - len;
-	dma_addr = dma_map_single(&ndev->dev, buffer, len, DMA_TO_DEVICE);
-	if (dma_mapping_error(&ndev->dev, dma_addr))
+	dma_addr = dma_map_single(ndev->dev.parent, buffer, len, DMA_TO_DEVICE);
+	if (dma_mapping_error(ndev->dev.parent, dma_addr))
 		goto unmap;
 
 	desc++;
@@ -1323,7 +1323,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
 		if (!ts_skb) {
 			desc--;
-			dma_unmap_single(&ndev->dev, dma_addr, len,
+			dma_unmap_single(ndev->dev.parent, dma_addr, len,
 					 DMA_TO_DEVICE);
 			goto unmap;
 		}
@@ -1358,7 +1358,7 @@ exit:
 	return NETDEV_TX_OK;
 
 unmap:
-	dma_unmap_single(&ndev->dev, le32_to_cpu(desc->dptr),
+	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
 			 le16_to_cpu(desc->ds_tagl), DMA_TO_DEVICE);
 drop:
 	dev_kfree_skb_any(skb);
@@ -1708,7 +1708,7 @@ static int ravb_probe(struct platform_device *pdev)
 
 	/* Allocate descriptor base address table */
 	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
-	priv->desc_bat = dma_alloc_coherent(NULL, priv->desc_bat_size,
+	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
 					    &priv->desc_bat_dma, GFP_KERNEL);
 	if (!priv->desc_bat) {
 		dev_err(&ndev->dev,
@@ -1763,7 +1763,7 @@ out_napi_del:
 	netif_napi_del(&priv->napi[RAVB_BE]);
 	ravb_mdio_release(priv);
 out_dma_free:
-	dma_free_coherent(NULL, priv->desc_bat_size, priv->desc_bat,
+	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 out_release:
 	if (ndev)
@@ -1779,7 +1779,7 @@ static int ravb_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct ravb_private *priv = netdev_priv(ndev);
 
-	dma_free_coherent(NULL, priv->desc_bat_size, priv->desc_bat,
+	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 	/* Set reset mode */
 	ravb_write(ndev, CCC_OPC_RESET, CCC);
-- 
2.1.4

[PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch updates the ravb binding to support the r8a7795 SoC by:
- Adding a compat string for the new hardware
- Adding 25 named interrupts to binding for the new SoC;
  older SoCs continue to use a single multiplexed interrupt

The example is also updated to reflect the r8a7795 as this is the
more complex case.

Based on work by Kazuya Mizuguchi and others.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---

v2
* First post; broken out of a driver update patch
* As discussed with Geert Uytterhoeven and Sergei Shtylyov
  - Binding: Make all interrupts mandatory as named-interrupts of
    the form ch%u
---
 .../devicetree/bindings/net/renesas,ravb.txt       | 65 +++++++++++++++++++---
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
index 1fd8831437bf..6c360f993d33 100644
--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
@@ -6,8 +6,11 @@ interface contains.
 Required properties:
 - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC.
 	      "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC.
+	      "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC.
 - reg: offset and length of (1) the register block and (2) the stream buffer.
-- interrupts: interrupt specifier for the sole interrupt.
+- interrupts: interrupt specifiers.
+	      One for each entry in interrupt-names the R8A7795 SoC;
+	      One entry for a multiplexed interrupt otherwise.
 - phy-mode: see ethernet.txt file in the same directory.
 - phy-handle: see ethernet.txt file in the same directory.
 - #address-cells: number of address cells for the MDIO bus, must be equal to 1.
@@ -18,6 +21,9 @@ Required properties:
 Optional properties:
 - interrupt-parent: the phandle for the interrupt controller that services
 		    interrupts for this device.
+- interrupt-names: One entry per interrupt named "ch%u".
+		   For the R8A7795 SoC this property is mandatory,
+		   and "ch0" through "ch24" are mandatory.
 - pinctrl-names: pin configuration state name ("default").
 - renesas,no-ether-link: boolean, specify when a board does not provide a proper
 			 AVB_LINK signal.
@@ -27,13 +33,46 @@ Optional properties:
 Example:
 
 	ethernet@e6800000 {
-		compatible = "renesas,etheravb-r8a7790";
-		reg = <0 0xe6800000 0 0x800>, <0 0xee0e8000 0 0x4000>;
+		compatible = "renesas,etheravb-r8a7795";
+		reg = <0 0xe6800000 0 0x800>, <0 0xe6a00000 0 0x10000>;
 		interrupt-parent = <&gic>;
-		interrupts = <0 163 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&mstp8_clks R8A7790_CLK_ETHERAVB>;
-		phy-mode = "rmii";
+		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "ch0", "ch1", "ch2", "ch3",
+				  "ch4", "ch5", "ch6", "ch7",
+				  "ch8", "ch9", "ch10", "ch11",
+				  "ch12", "ch13", "ch14", "ch15",
+				  "ch16", "ch17", "ch18", "ch19",
+				  "ch20", "ch21", "ch22", "ch23",
+				  "ch24";
+		clocks = <&mstp8_clks R8A7795_CLK_ETHERAVB>;
+		phy-mode = "rgmii-id";
+		phy-reset-gpio = <&gpio2 10 0>;
 		phy-handle = <&phy0>;
+
 		pinctrl-0 = <&ether_pins>;
 		pinctrl-names = "default";
 		renesas,no-ether-link;
@@ -41,8 +80,20 @@ Example:
 		#size-cells = <0>;
 
 		phy0: ethernet-phy@0 {
+			rxc-skew-ps = <900>;
+			rxdv-skew-ps = <0>;
+			rxd0-skew-ps = <0>;
+			rxd1-skew-ps = <0>;
+			rxd2-skew-ps = <0>;
+			rxd3-skew-ps = <0>;
+			txc-skew-ps = <900>;
+			txen-skew-ps = <0>;
+			txd0-skew-ps = <0>;
+			txd1-skew-ps = <0>;
+			txd2-skew-ps = <0>;
+			txd3-skew-ps = <0>;
 			reg = <0>;
 			interrupt-parent = <&gpio2>;
-			interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
 		};
 	};
-- 
2.1.4

[PATCH/RFC v2 net-next 4/4] ravb: Add support for r8a7795 SoC

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch supports the r8a7795 SoC by:
- Using two interrupts
  + One for E-MAC
  + One for everything else
  + Both can be handled by the existing common interrupt handler, which
    affords a simpler update to support the new SoC. In future some
    consideration may be given to implementing multiple interrupt handlers
- Limiting the phy speed to 100Mbit/s for the new SoC;
  at this time it is not clear how this restriction may be lifted
  but I hope it will be possible as more information comes to light

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
[horms: reworked]
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---

v0 [Kazuya Mizuguchi]

v1 [Simon Horman]
* Updated patch subject

v2 [Simon Horman]
* Reworked based on extensive feedback from
  Geert Uytterhoeven and Sergei Shtylyov.
* Broke binding update out into separate patch
---
 drivers/net/ethernet/renesas/ravb.h      |  7 ++++
 drivers/net/ethernet/renesas/ravb_main.c | 57 +++++++++++++++++++++++++++-----
 2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a157aaaaff6a..0623fff932e4 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -766,6 +766,11 @@ struct ravb_ptp {
 	struct ravb_ptp_perout perout[N_PER_OUT];
 };
 
+enum ravb_chip_id {
+	RCAR_GEN2,
+	RCAR_GEN3,
+};
+
 struct ravb_private {
 	struct net_device *ndev;
 	struct platform_device *pdev;
@@ -806,6 +811,8 @@ struct ravb_private {
 	int msg_enable;
 	int speed;
 	int duplex;
+	int emac_irq;
+	enum ravb_chip_id chip_id;
 
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4ca093d033f8..cc6702e33d65 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -889,6 +889,14 @@ static int ravb_phy_init(struct net_device *ndev)
 		return -ENOENT;
 	}
 
+	/* This driver only support 10/100Mbit speeds on Gen3
+	 * at this time.
+	 */
+	if (priv->chip_id == RCAR_GEN3) {
+		netdev_info(ndev, "limiting PHY to 100Mbit/s\n");
+		phy_set_max_speed(phydev, SPEED_100);
+	}
+
 	netdev_info(ndev, "attached PHY %d (IRQ %d) to driver %s\n",
 		    phydev->addr, phydev->irq, phydev->drv->name);
 
@@ -1197,6 +1205,15 @@ static int ravb_open(struct net_device *ndev)
 		goto out_napi_off;
 	}
 
+	if (priv->chip_id == RCAR_GEN3) {
+		error = request_irq(priv->emac_irq, ravb_interrupt,
+				    IRQF_SHARED, ndev->name, ndev);
+		if (error) {
+			netdev_err(ndev, "cannot request IRQ\n");
+			goto out_free_irq;
+		}
+	}
+
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
@@ -1220,6 +1237,7 @@ out_ptp_stop:
 	ravb_ptp_stop(ndev);
 out_free_irq:
 	free_irq(ndev->irq, ndev);
+	free_irq(priv->emac_irq, ndev);
 out_napi_off:
 	napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -1625,13 +1643,23 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static const struct of_device_id ravb_match_table[] = {
+	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
+	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
+	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ravb_match_table);
+
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct ravb_private *priv;
+	enum ravb_chip_id chip_id;
 	struct net_device *ndev;
-	int error, irq, q;
 	struct resource *res;
+	int error, irq, q;
 
 	if (!np) {
 		dev_err(&pdev->dev,
@@ -1657,7 +1685,14 @@ static int ravb_probe(struct platform_device *pdev)
 	/* The Ether-specific entries in the device structure. */
 	ndev->base_addr = res->start;
 	ndev->dma = -1;
-	irq = platform_get_irq(pdev, 0);
+
+	match = of_match_device(of_match_ptr(ravb_match_table), &pdev->dev);
+	chip_id = (enum ravb_chip_id)match->data;
+
+	if (chip_id == RCAR_GEN3)
+		irq = platform_get_irq_byname(pdev, "ch22");
+	else
+		irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		error = irq;
 		goto out_release;
@@ -1688,6 +1723,17 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
+	if (chip_id == RCAR_GEN3) {
+		irq = platform_get_irq_byname(pdev, "ch24");
+		if (irq < 0) {
+			error = irq;
+			goto out_release;
+		}
+		priv->emac_irq = irq;
+	}
+
+	priv->chip_id = chip_id;
+
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
@@ -1818,13 +1864,6 @@ static const struct dev_pm_ops ravb_dev_pm_ops = {
 #define RAVB_PM_OPS NULL
 #endif
 
-static const struct of_device_id ravb_match_table[] = {
-	{ .compatible = "renesas,etheravb-r8a7790" },
-	{ .compatible = "renesas,etheravb-r8a7794" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, ravb_match_table);
-
 static struct platform_driver ravb_driver = {
 	.probe		= ravb_probe,
 	.remove		= ravb_remove,
-- 
2.1.4

Re: [PATCH/RFC v2 net-next 1/4] phylib: Add phy_set_max_speed helper

[Florian Fainelli]

Le 09/10/15 19:01, Simon Horman a écrit :
> Add a helper to allow ethernet drivers to limit the speed of a phy
> (that they are attached to).
> 
> This mainly involves factoring out the business-end of
> of_set_phy_supported() and exporting a new symbol.
> 
> This code seems to be open coded in several places, in several different
> variants.
> 
> This code is envisaged this will be used in situations where setting
> the "max-speed" property is not appropriate, e.g. because the maximum
> speed is not a property of the phy hardware.

This looks good to me, one minor comment, see below:

> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
> ---
> 
> v2
> * First post
> ---
>  drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++----------------
>  include/linux/phy.h          |  1 +
>  2 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c0f211127274..d9a020095972 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
> +{
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return;

I think that part should be moved to of_set_phy_supported(), since your
are exporting phy_set_max_speed() which should therefore be available
regardless of whether Device Tree is used.

While you are it, it might be nice to either warn or return -ENOTSUPP if
the speed does not match 10, 100 or 1000, but that might be worth a
second patch.

Thanks!
-- 
Florian

Re: [PATCH/RFC v2 net-next 1/4] phylib: Add phy_set_max_speed helper

[Simon Horman]

On Thu, Sep 10, 2015 at 09:14:34PM -0700, Florian Fainelli wrote:
> Le 09/10/15 19:01, Simon Horman a écrit :
> > Add a helper to allow ethernet drivers to limit the speed of a phy
> > (that they are attached to).
> > 
> > This mainly involves factoring out the business-end of
> > of_set_phy_supported() and exporting a new symbol.
> > 
> > This code seems to be open coded in several places, in several different
> > variants.
> > 
> > This code is envisaged this will be used in situations where setting
> > the "max-speed" property is not appropriate, e.g. because the maximum
> > speed is not a property of the phy hardware.
> 
> This looks good to me, one minor comment, see below:
> 
> > 
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > 
> > ---
> > 
> > v2
> > * First post
> > ---
> >  drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++----------------
> >  include/linux/phy.h          |  1 +
> >  2 files changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index c0f211127274..d9a020095972 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
> > +{
> > +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> > +		return;
> 
> I think that part should be moved to of_set_phy_supported(), since your
> are exporting phy_set_max_speed() which should therefore be available
> regardless of whether Device Tree is used.

Yes of course, silly me.

> While you are it, it might be nice to either warn or return -ENOTSUPP if
> the speed does not match 10, 100 or 1000, but that might be worth a
> second patch.

Sure, that sounds reasonable.

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Geert Uytterhoeven]

Hi Simon,

On Fri, Sep 11, 2015 at 4:01 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -6,8 +6,11 @@ interface contains.
>  Required properties:
>  - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC.
>               "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC.
> +             "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC.
>  - reg: offset and length of (1) the register block and (2) the stream buffer.
> -- interrupts: interrupt specifier for the sole interrupt.
> +- interrupts: interrupt specifiers.
> +             One for each entry in interrupt-names the R8A7795 SoC;

... for the R8A7795 SoC

> +             One entry for a multiplexed interrupt otherwise.
>  - phy-mode: see ethernet.txt file in the same directory.
>  - phy-handle: see ethernet.txt file in the same directory.
>  - #address-cells: number of address cells for the MDIO bus, must be equal to 1.
> @@ -18,6 +21,9 @@ Required properties:
>  Optional properties:
>  - interrupt-parent: the phandle for the interrupt controller that services
>                     interrupts for this device.
> +- interrupt-names: One entry per interrupt named "ch%u".
> +                  For the R8A7795 SoC this property is mandatory,
> +                  and "ch0" through "ch24" are mandatory.

This suggests the single multiplexed interrupt on R-Car Gen2 can be called
"ch0". Is that what you want? I know the driver doesn't care.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Simon Horman]

On Fri, Sep 11, 2015 at 09:12:17AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Sep 11, 2015 at 4:01 AM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > @@ -6,8 +6,11 @@ interface contains.
> >  Required properties:
> >  - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC.
> >               "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC.
> > +             "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC.
> >  - reg: offset and length of (1) the register block and (2) the stream buffer.
> > -- interrupts: interrupt specifier for the sole interrupt.
> > +- interrupts: interrupt specifiers.
> > +             One for each entry in interrupt-names the R8A7795 SoC;
> 
> ... for the R8A7795 SoC
> 
> > +             One entry for a multiplexed interrupt otherwise.
> >  - phy-mode: see ethernet.txt file in the same directory.
> >  - phy-handle: see ethernet.txt file in the same directory.
> >  - #address-cells: number of address cells for the MDIO bus, must be equal to 1.
> > @@ -18,6 +21,9 @@ Required properties:
> >  Optional properties:
> >  - interrupt-parent: the phandle for the interrupt controller that services
> >                     interrupts for this device.
> > +- interrupt-names: One entry per interrupt named "ch%u".
> > +                  For the R8A7795 SoC this property is mandatory,
> > +                  and "ch0" through "ch24" are mandatory.
> 
> This suggests the single multiplexed interrupt on R-Car Gen2 can be called
> "ch0". Is that what you want? I know the driver doesn't care.

No, its not what I intended.

I think its reasonable to allow the multiplexed interrupt to be named,
but to what I wonder. The documentation seems to call the interrupt
"EthernetAVB", which isn't very exciting.

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Geert Uytterhoeven]

Hi Simon,

On Fri, Sep 11, 2015 at 10:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> > @@ -18,6 +21,9 @@ Required properties:
>> >  Optional properties:
>> >  - interrupt-parent: the phandle for the interrupt controller that services
>> >                     interrupts for this device.
>> > +- interrupt-names: One entry per interrupt named "ch%u".
>> > +                  For the R8A7795 SoC this property is mandatory,
>> > +                  and "ch0" through "ch24" are mandatory.
>>
>> This suggests the single multiplexed interrupt on R-Car Gen2 can be called
>> "ch0". Is that what you want? I know the driver doesn't care.
>
> No, its not what I intended.
>
> I think its reasonable to allow the multiplexed interrupt to be named,
> but to what I wonder. The documentation seems to call the interrupt
> "EthernetAVB", which isn't very exciting.

Perhaps "mux", like I did for rspi, cfr.
Documentation/devicetree/bindings/spi/spi-rspi.txt:

  - interrupts       : A list of interrupt-specifiers, one for each entry in
                       interrupt-names.
                       If interrupt-names is not present, an interrupt specifier
                       for a single muxed interrupt.
  - interrupt-names  : A list of interrupt names. Should contain (if present):
                         - "error" for SPEI,
                         - "rx" for SPRI,
                         - "tx" to SPTI,
                         - "mux" for a single muxed interrupt.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Simon Horman]

On Fri, Sep 11, 2015 at 10:41:31AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Sep 11, 2015 at 10:14 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > @@ -18,6 +21,9 @@ Required properties:
> >> >  Optional properties:
> >> >  - interrupt-parent: the phandle for the interrupt controller that services
> >> >                     interrupts for this device.
> >> > +- interrupt-names: One entry per interrupt named "ch%u".
> >> > +                  For the R8A7795 SoC this property is mandatory,
> >> > +                  and "ch0" through "ch24" are mandatory.
> >>
> >> This suggests the single multiplexed interrupt on R-Car Gen2 can be called
> >> "ch0". Is that what you want? I know the driver doesn't care.
> >
> > No, its not what I intended.
> >
> > I think its reasonable to allow the multiplexed interrupt to be named,
> > but to what I wonder. The documentation seems to call the interrupt
> > "EthernetAVB", which isn't very exciting.
> 
> Perhaps "mux", like I did for rspi, cfr.
> Documentation/devicetree/bindings/spi/spi-rspi.txt:
> 
>   - interrupts       : A list of interrupt-specifiers, one for each entry in
>                        interrupt-names.
>                        If interrupt-names is not present, an interrupt specifier
>                        for a single muxed interrupt.
>   - interrupt-names  : A list of interrupt names. Should contain (if present):
>                          - "error" for SPEI,
>                          - "rx" for SPRI,
>                          - "tx" to SPTI,
>                          - "mux" for a single muxed interrupt.

Thanks for for that example. "mux" sounds good to me.

Re: [PATCH/RFC v2 net-next 1/4] phylib: Add phy_set_max_speed helper

[Sergei Shtylyov]

Hello.

On 9/11/2015 5:01 AM, Simon Horman wrote:

> Add a helper to allow ethernet drivers to limit the speed of a phy
> (that they are attached to).
>
> This mainly involves factoring out the business-end of
> of_set_phy_supported() and exporting a new symbol.
>
> This code seems to be open coded in several places, in several different
> variants.
>
> This code is envisaged this will be used in situations where setting
> the "max-speed" property is not appropriate, e.g. because the maximum
> speed is not a property of the phy hardware.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
>
> v2
> * First post
> ---
>   drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++----------------
>   include/linux/phy.h          |  1 +
>   2 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c0f211127274..d9a020095972 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev)
>   	return 0;
>   }
>
> +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
> +{
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return;
> +
> +	/* The default values for phydev->supported are provided by the PHY
> +	 * driver "features" member, we want to reset to sane defaults fist

    s/fist/first/.

> +	 * before supporting higher speeds.
> +	 */
> +	phydev->supported &= PHY_DEFAULT_FEATURES;
> +
> +	switch (max_speed) {
> +	default:
> +		return;
> +

    I don't think empty line is needed here.

> +	case SPEED_1000:
> +		phydev->supported |= PHY_1000BT_FEATURES;

    Need a comment like /* Fall thru */.

> +	case SPEED_100:
> +		phydev->supported |= PHY_100BT_FEATURES;

    And here.

> +	case SPEED_10:
> +		phydev->supported |= PHY_10BT_FEATURES;
> +	}
> +}
[...]

MBR, Sergei

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Sergei Shtylyov]

Hello.

On 9/11/2015 5:01 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> This patch updates the ravb binding to support the r8a7795 SoC by:
> - Adding a compat string for the new hardware
> - Adding 25 named interrupts to binding for the new SoC;
>    older SoCs continue to use a single multiplexed interrupt
>
> The example is also updated to reflect the r8a7795 as this is the
> more complex case.
>
> Based on work by Kazuya Mizuguchi and others.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
>
> v2
> * First post; broken out of a driver update patch
> * As discussed with Geert Uytterhoeven and Sergei Shtylyov
>    - Binding: Make all interrupts mandatory as named-interrupts of
>      the form ch%u
> ---
>   .../devicetree/bindings/net/renesas,ravb.txt       | 65 +++++++++++++++++++---
>   1 file changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index 1fd8831437bf..6c360f993d33 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
[...]
> @@ -27,13 +33,46 @@ Optional properties:
>   Example:
>
>   	ethernet@e6800000 {
> -		compatible = "renesas,etheravb-r8a7790";
> -		reg = <0 0xe6800000 0 0x800>, <0 0xee0e8000 0 0x4000>;
> +		compatible = "renesas,etheravb-r8a7795";
> +		reg = <0 0xe6800000 0 0x800>, <0 0xe6a00000 0 0x10000>;
>   		interrupt-parent = <&gic>;
> -		interrupts = <0 163 IRQ_TYPE_LEVEL_HIGH>;
> -		clocks = <&mstp8_clks R8A7790_CLK_ETHERAVB>;
> -		phy-mode = "rmii";
> +		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "ch0", "ch1", "ch2", "ch3",
> +				  "ch4", "ch5", "ch6", "ch7",
> +				  "ch8", "ch9", "ch10", "ch11",
> +				  "ch12", "ch13", "ch14", "ch15",
> +				  "ch16", "ch17", "ch18", "ch19",
> +				  "ch20", "ch21", "ch22", "ch23",
> +				  "ch24";

    To me, these names don't look very helpful. You could as well omit them 
and use platform_get_irq() with the channel #.

> +		clocks = <&mstp8_clks R8A7795_CLK_ETHERAVB>;
> +		phy-mode = "rgmii-id";
> +		phy-reset-gpio = <&gpio2 10 0>;

    I don't see how this prop is used by the driver and it's not documented in 
the bindings.

[...]

MBR, Sergei

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Simon Horman]

On Fri, Sep 11, 2015 at 05:25:17PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 9/11/2015 5:01 AM, Simon Horman wrote:
> 
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >
> >This patch updates the ravb binding to support the r8a7795 SoC by:
> >- Adding a compat string for the new hardware
> >- Adding 25 named interrupts to binding for the new SoC;
> >   older SoCs continue to use a single multiplexed interrupt
> >
> >The example is also updated to reflect the r8a7795 as this is the
> >more complex case.
> >
> >Based on work by Kazuya Mizuguchi and others.
> >
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> >---
> >
> >v2
> >* First post; broken out of a driver update patch
> >* As discussed with Geert Uytterhoeven and Sergei Shtylyov
> >   - Binding: Make all interrupts mandatory as named-interrupts of
> >     the form ch%u
> >---
> >  .../devicetree/bindings/net/renesas,ravb.txt       | 65 +++++++++++++++++++---
> >  1 file changed, 58 insertions(+), 7 deletions(-)
> >
> >diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >index 1fd8831437bf..6c360f993d33 100644
> >--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> [...]
> >@@ -27,13 +33,46 @@ Optional properties:
> >  Example:
> >
> >  	ethernet@e6800000 {
> >-		compatible = "renesas,etheravb-r8a7790";
> >-		reg = <0 0xe6800000 0 0x800>, <0 0xee0e8000 0 0x4000>;
> >+		compatible = "renesas,etheravb-r8a7795";
> >+		reg = <0 0xe6800000 0 0x800>, <0 0xe6a00000 0 0x10000>;
> >  		interrupt-parent = <&gic>;
> >-		interrupts = <0 163 IRQ_TYPE_LEVEL_HIGH>;
> >-		clocks = <&mstp8_clks R8A7790_CLK_ETHERAVB>;
> >-		phy-mode = "rmii";
> >+		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
> >+			     <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> >+		interrupt-names = "ch0", "ch1", "ch2", "ch3",
> >+				  "ch4", "ch5", "ch6", "ch7",
> >+				  "ch8", "ch9", "ch10", "ch11",
> >+				  "ch12", "ch13", "ch14", "ch15",
> >+				  "ch16", "ch17", "ch18", "ch19",
> >+				  "ch20", "ch21", "ch22", "ch23",
> >+				  "ch24";
> 
>    To me, these names don't look very helpful. You could as well omit them
> and use platform_get_irq() with the channel #.

These names reflect the hardware; which is the aim of DT.

As I believe you pointed out earlier it is preferred to use named
interrupts when there is more than one. Do I misunderstand the situation
there?

If you have a positive contribution to make regarding better names then
I am all ears.

> >+		clocks = <&mstp8_clks R8A7795_CLK_ETHERAVB>;
> >+		phy-mode = "rgmii-id";
> >+		phy-reset-gpio = <&gpio2 10 0>;
> 
>    I don't see how this prop is used by the driver and it's not documented
> in the bindings.

Thanks for pointing that out. It looks like I should remove phy-reset-gpio.

Re: [PATCH/RFC v2 net-next 3/4] ravb: Document binding for r8a7795 SoC

[Sergei Shtylyov]

Hello.

On 09/14/2015 03:42 AM, Simon Horman wrote:

    Sorry for delayed reply, I thought I'd already replied to this. :-/

>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>
>>> This patch updates the ravb binding to support the r8a7795 SoC by:
>>> - Adding a compat string for the new hardware
>>> - Adding 25 named interrupts to binding for the new SoC;
>>>    older SoCs continue to use a single multiplexed interrupt
>>>
>>> The example is also updated to reflect the r8a7795 as this is the
>>> more complex case.
>>>
>>> Based on work by Kazuya Mizuguchi and others.
>>>
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>
>>> ---
>>>
>>> v2
>>> * First post; broken out of a driver update patch
>>> * As discussed with Geert Uytterhoeven and Sergei Shtylyov
>>>    - Binding: Make all interrupts mandatory as named-interrupts of
>>>      the form ch%u
>>> ---
>>>   .../devicetree/bindings/net/renesas,ravb.txt       | 65 +++++++++++++++++++---
>>>   1 file changed, 58 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> index 1fd8831437bf..6c360f993d33 100644
>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>> [...]
>>> @@ -27,13 +33,46 @@ Optional properties:
>>>   Example:
>>>
>>>   	ethernet@e6800000 {
>>> -		compatible = "renesas,etheravb-r8a7790";
>>> -		reg = <0 0xe6800000 0 0x800>, <0 0xee0e8000 0 0x4000>;
>>> +		compatible = "renesas,etheravb-r8a7795";
>>> +		reg = <0 0xe6800000 0 0x800>, <0 0xe6a00000 0 0x10000>;
>>>   		interrupt-parent = <&gic>;
>>> -		interrupts = <0 163 IRQ_TYPE_LEVEL_HIGH>;
>>> -		clocks = <&mstp8_clks R8A7790_CLK_ETHERAVB>;
>>> -		phy-mode = "rmii";
>>> +		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
>>> +			     <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
>>> +		interrupt-names = "ch0", "ch1", "ch2", "ch3",
>>> +				  "ch4", "ch5", "ch6", "ch7",
>>> +				  "ch8", "ch9", "ch10", "ch11",
>>> +				  "ch12", "ch13", "ch14", "ch15",
>>> +				  "ch16", "ch17", "ch18", "ch19",
>>> +				  "ch20", "ch21", "ch22", "ch23",
>>> +				  "ch24";
>>
>>     To me, these names don't look very helpful. You could as well omit them
>> and use platform_get_irq() with the channel #.

> These names reflect the hardware; which is the aim of DT.

    Indeed (I've looked into the manuals by now). They just look poorly 
chosen. :-)

> As I believe you pointed out earlier it is preferred to use named
> interrupts when there is more than one. Do I misunderstand the situation
> there?

    Yes.

> If you have a positive contribution to make regarding better names then
> I am all ears.

    I liked your "tx<n>", "rx<n>" variant better...

MBR, Sergei