[PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

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

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
[horms: updated changelog]
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 .../devicetree/bindings/net/renesas,ravb.txt       |  6 ++-
 drivers/net/ethernet/renesas/ravb.h                |  1 +
 drivers/net/ethernet/renesas/ravb_main.c           | 47 +++++++++++++++++++---
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
index 1fd8831437bf..0b4ec02c35a4 100644
--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
@@ -6,8 +6,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
+              interrupt specifier for the sole interrupt.
+              if the device is a part of R8A7795 SoC
+              interrupt specifier for the two interrupts.
 - 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.
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a157aaaaff6a..1832737063f3 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -809,6 +809,7 @@ struct ravb_private {
 
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
+	int emac_irq;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 026d98435d87..bf604a869458 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = {
 static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct platform_device *pdev = priv->pdev;
+	struct device *dev = &pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
 	napi_enable(&priv->napi[RAVB_NC]);
 
-	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
-			    ndev);
-	if (error) {
-		netdev_err(ndev, "cannot request IRQ\n");
-		goto out_napi_off;
+	if (of_device_is_compatible(dev->of_node,
+				"renesas,etheravb-r8a7795")) {
+		error = request_irq(ndev->irq,
+				ravb_interrupt, IRQF_SHARED, ndev->name, ndev);
+		if (error) {
+			netdev_err(ndev, "cannot request IRQ\n");
+			goto out_napi_off;
+		}
+		error = request_irq(priv->emac_irq,
+				ravb_interrupt,	IRQF_SHARED, ndev->name, ndev);
+		if (error) {
+			netdev_err(ndev, "cannot request IRQ\n");
+			free_irq(ndev->irq, ndev);
+			goto out_napi_off;
+		}
+	}
+	else {
+		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
+				ndev->name, ndev);
+		if (error) {
+			netdev_err(ndev, "cannot request IRQ\n");
+			goto out_napi_off;
+		}
 	}
 
 	/* Device init */
@@ -1219,7 +1239,11 @@ out_ptp_stop:
 	/* Stop PTP Clock driver */
 	ravb_ptp_stop(ndev);
 out_free_irq:
-	free_irq(ndev->irq, ndev);
+	if (of_device_is_compatible(dev->of_node,
+				"renesas,etheravb-r8a7795"))
+		free_irq(priv->emac_irq, ndev);
+	else
+		free_irq(ndev->irq, ndev);
 out_napi_off:
 	napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low  		of_property_read_bool(np, "renesas,ether-link-active-low");
 
+	if (of_device_is_compatible(np,
+				"renesas,etheravb-r8a7795")) {
+		irq = platform_get_irq(pdev, 1);
+		if (irq < 0) {
+			error = -ENODEV;
+			goto out_release;
+		}
+		priv->emac_irq = irq;
+	}
+
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
@@ -1821,6 +1855,7 @@ static const struct dev_pm_ops ravb_dev_pm_ops = {
 static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790" },
 	{ .compatible = "renesas,etheravb-r8a7794" },
+	{ .compatible = "renesas,etheravb-r8a7795" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
-- 
2.1.4

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Geert Uytterhoeven]

On Thu, Aug 27, 2015 at 11:24 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,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> +              interrupt specifier for the sole interrupt.
> +              if the device is a part of R8A7795 SoC
> +              interrupt specifier for the two interrupts.

If there are multiple interrupts, you best make them named interrupts,
i.e. requiring "interrupt-names", too.

Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24.

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 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 8/27/2015 12:24 PM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> [horms: updated changelog]

    I don't see any. ;-)

> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>   .../devicetree/bindings/net/renesas,ravb.txt       |  6 ++-
>   drivers/net/ethernet/renesas/ravb.h                |  1 +
>   drivers/net/ethernet/renesas/ravb_main.c           | 47 +++++++++++++++++++---
>   3 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index 1fd8831437bf..0b4ec02c35a4 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -6,8 +6,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> +              interrupt specifier for the sole interrupt.
> +              if the device is a part of R8A7795 SoC

    "If" since it starts the statement?

> +              interrupt specifier for the two interrupts.

    You need to be more verbose here: what two interrupts? Perhaps need to use 
"interrupt-names"?

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index a157aaaaff6a..1832737063f3 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -809,6 +809,7 @@ struct ravb_private {
>
>   	unsigned no_avb_link:1;
>   	unsigned avb_link_active_low:1;
> +	int emac_irq;
>   };

    Why not add it above the bit fields?

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 026d98435d87..bf604a869458 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>   static int ravb_open(struct net_device *ndev)
>   {
>   	struct ravb_private *priv = netdev_priv(ndev);
> +	struct platform_device *pdev = priv->pdev;

    You hardly need this variable.

> +	struct device *dev = &pdev->dev;
>   	int error;
>
>   	napi_enable(&priv->napi[RAVB_BE]);
>   	napi_enable(&priv->napi[RAVB_NC]);
>
> -	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
> -			    ndev);
> -	if (error) {
> -		netdev_err(ndev, "cannot request IRQ\n");
> -		goto out_napi_off;
> +	if (of_device_is_compatible(dev->of_node,

    Are you going to add more calls as the support for more SoCs get added?

> +				"renesas,etheravb-r8a7795")) {

    Please respect the networking code alignment rules, start this line right 
under 'dev'.

> +		error = request_irq(ndev->irq,
> +				ravb_interrupt, IRQF_SHARED, ndev->name, ndev);

    Likewise.

> +		if (error) {
> +			netdev_err(ndev, "cannot request IRQ\n");
> +			goto out_napi_off;
> +		}
> +		error = request_irq(priv->emac_irq,
> +				ravb_interrupt,	IRQF_SHARED, ndev->name, ndev);

    Likewise.
    And using the same handler for both interrupts doesn't look good.

> +		if (error) {
> +			netdev_err(ndev, "cannot request IRQ\n");
> +			free_irq(ndev->irq, ndev);
> +			goto out_napi_off;
> +		}
> +	}
> +	else {
> +		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
> +				ndev->name, ndev);

    Please respect the alignment rules.

> +		if (error) {
> +			netdev_err(ndev, "cannot request IRQ\n");
> +			goto out_napi_off;
> +		}

    I don't understand why you have to duplicate this call in both branches.

>   	}
>
>   	/* Device init */
> @@ -1219,7 +1239,11 @@ out_ptp_stop:
>   	/* Stop PTP Clock driver */
>   	ravb_ptp_stop(ndev);
>   out_free_irq:
> -	free_irq(ndev->irq, ndev);
> +	if (of_device_is_compatible(dev->of_node,
> +				"renesas,etheravb-r8a7795"))
> +		free_irq(priv->emac_irq, ndev);

    What about the AVB-DMAC interrupt in this case?

> +	else
> +		free_irq(ndev->irq, ndev);

    This should be done regardless.

[...]
> @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
>   	priv->avb_link_active_low >   		of_property_read_bool(np, "renesas,ether-link-active-low");
>
> +	if (of_device_is_compatible(np,
> +				"renesas,etheravb-r8a7795")) {

    Please respect the rules. And do you really need the break the line here?

> +		irq = platform_get_irq(pdev, 1);
> +		if (irq < 0) {
> +			error = -ENODEV;

    Please propagate the error instead.

> +			goto out_release;
> +		}
> +		priv->emac_irq = irq;
> +	}
> +
[...]

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Thu, Aug 27, 2015 at 01:01:50PM +0200, Geert Uytterhoeven wrote:
> On Thu, Aug 27, 2015 at 11:24 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,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> > +              interrupt specifier for the sole interrupt.
> > +              if the device is a part of R8A7795 SoC
> > +              interrupt specifier for the two interrupts.
> 
> If there are multiple interrupts, you best make them named interrupts,
> i.e. requiring "interrupt-names", too.

Thanks, I will look into that.

> Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24.

Thanks for bringing that up. I think it has also come up elsewhere in this
thread. I'd like to focus on addressing it here rather than spreading the
discussion around any further.

My understanding is that on the R-Car Gen3 r8a7795 SoC
the EthernetAVB hardware may function in one of two modes.

1. A mode which is "mostly" compatible with R-Car Gen2
   (e.g. r8a7790, r8a7790).

   In this mode only ch22 and ch24 are used.

   I note that the emac interrupt also appears to be documented for Gen-2.
   Its not clear to me at this time why it isn't appropriate to use it
   on those SoCs.

2. A mode which is new in Gen 3.

   In this mode ch0 - ch24 are used.
   I believe that in this mode there are per DMA queue interrupts.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Thu, Aug 27, 2015 at 02:03:37PM +0300, Sergei Shtylyov wrote:
> On 8/27/2015 12:24 PM, Simon Horman wrote:
> 
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >
> >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >[horms: updated changelog]
> 
>    I don't see any. ;-)
> 
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >---
> >  .../devicetree/bindings/net/renesas,ravb.txt       |  6 ++-
> >  drivers/net/ethernet/renesas/ravb.h                |  1 +
> >  drivers/net/ethernet/renesas/ravb_main.c           | 47 +++++++++++++++++++---
> >  3 files changed, 47 insertions(+), 7 deletions(-)
> >
> >diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >index 1fd8831437bf..0b4ec02c35a4 100644
> >--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >@@ -6,8 +6,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> >+              interrupt specifier for the sole interrupt.
> >+              if the device is a part of R8A7795 SoC
> 
>    "If" since it starts the statement?
> 
> >+              interrupt specifier for the two interrupts.
> 
>    You need to be more verbose here: what two interrupts? Perhaps need to
> use "interrupt-names"?

Thanks, I will look into that.

> [...]
> >diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >index a157aaaaff6a..1832737063f3 100644
> >--- a/drivers/net/ethernet/renesas/ravb.h
> >+++ b/drivers/net/ethernet/renesas/ravb.h
> >@@ -809,6 +809,7 @@ struct ravb_private {
> >
> >  	unsigned no_avb_link:1;
> >  	unsigned avb_link_active_low:1;
> >+	int emac_irq;
> >  };
> 
>    Why not add it above the bit fields?

Using an int seems reasonable to me as it is used to hold
an integer value returned by request_irq().

> [...]
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index 026d98435d87..bf604a869458 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >@@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = {
> >  static int ravb_open(struct net_device *ndev)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >+	struct platform_device *pdev = priv->pdev;
> 
>    You hardly need this variable.
> 
> >+	struct device *dev = &pdev->dev;
> >  	int error;
> >
> >  	napi_enable(&priv->napi[RAVB_BE]);
> >  	napi_enable(&priv->napi[RAVB_NC]);
> >
> >-	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
> >-			    ndev);
> >-	if (error) {
> >-		netdev_err(ndev, "cannot request IRQ\n");
> >-		goto out_napi_off;
> >+	if (of_device_is_compatible(dev->of_node,
> 
>    Are you going to add more calls as the support for more SoCs get added?

I think it is reasonable to assume that is likely
that support for more SoCs will be added.
However, I am not aware of any immediate plans to do so.

> 
> >+				"renesas,etheravb-r8a7795")) {
> 
>    Please respect the networking code alignment rules, start this line right
> under 'dev'.

Sure, will do. Likewise elsewhere in this patch.

> >+		error = request_irq(ndev->irq,
> >+				ravb_interrupt, IRQF_SHARED, ndev->name, ndev);
> 
>    Likewise.
> 
> >+		if (error) {
> >+			netdev_err(ndev, "cannot request IRQ\n");
> >+			goto out_napi_off;
> >+		}
> >+		error = request_irq(priv->emac_irq,
> >+				ravb_interrupt,	IRQF_SHARED, ndev->name, ndev);
> 
>    Likewise.
>    And using the same handler for both interrupts doesn't look good.

It does seem a little odd. I will confirm that detail.

> >+		if (error) {
> >+			netdev_err(ndev, "cannot request IRQ\n");
> >+			free_irq(ndev->irq, ndev);
> >+			goto out_napi_off;
> >+		}
> >+	}
> >+	else {
> >+		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
> >+				ndev->name, ndev);
> 
>    Please respect the alignment rules.
> 
> >+		if (error) {
> >+			netdev_err(ndev, "cannot request IRQ\n");
> >+			goto out_napi_off;
> >+		}
> 
>    I don't understand why you have to duplicate this call in both branches.

Thanks, I'll see about cleaning that up.

> >  	}
> >
> >  	/* Device init */
> >@@ -1219,7 +1239,11 @@ out_ptp_stop:
> >  	/* Stop PTP Clock driver */
> >  	ravb_ptp_stop(ndev);
> >  out_free_irq:
> >-	free_irq(ndev->irq, ndev);
> >+	if (of_device_is_compatible(dev->of_node,
> >+				"renesas,etheravb-r8a7795"))
> >+		free_irq(priv->emac_irq, ndev);
> 
>    What about the AVB-DMAC interrupt in this case?
> 
> >+	else
> >+		free_irq(ndev->irq, ndev);
> 
>    This should be done regardless.

Thanks, that looks wrong.
I'll see about fixing it.

> [...]
> >@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
> >  	priv->avb_link_active_low > >  		of_property_read_bool(np, "renesas,ether-link-active-low");
> >
> >+	if (of_device_is_compatible(np,
> >+				"renesas,etheravb-r8a7795")) {
> 
>    Please respect the rules. And do you really need the break the line here?
> 
> >+		irq = platform_get_irq(pdev, 1);
> >+		if (irq < 0) {
> >+			error = -ENODEV;
> 
>    Please propagate the error instead.

Sure, will do.

> 
> >+			goto out_release;
> >+		}
> >+		priv->emac_irq = irq;
> >+	}
> >+
> [...]
> 
> MBR, Sergei
> 

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 11:01:20AM +0900, Simon Horman wrote:
> On Thu, Aug 27, 2015 at 02:03:37PM +0300, Sergei Shtylyov wrote:
> > On 8/27/2015 12:24 PM, Simon Horman wrote:

[snip]

> > >@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
> > >  	priv->avb_link_active_low > > >  		of_property_read_bool(np, "renesas,ether-link-active-low");
> > >
> > >+	if (of_device_is_compatible(np,
> > >+				"renesas,etheravb-r8a7795")) {
> > 
> >    Please respect the rules. And do you really need the break the line here?
> > 
> > >+		irq = platform_get_irq(pdev, 1);
> > >+		if (irq < 0) {
> > >+			error = -ENODEV;
> > 
> >    Please propagate the error instead.
> 
> Sure, will do.

On closer examination the above is consistent with the existing
error handling for another call to platform_get_irq() in the same function.
Shall we fix that first?

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 10:42:37AM +0900, Simon Horman wrote:
> On Thu, Aug 27, 2015 at 01:01:50PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Aug 27, 2015 at 11:24 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,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> > > +              interrupt specifier for the sole interrupt.
> > > +              if the device is a part of R8A7795 SoC
> > > +              interrupt specifier for the two interrupts.
> > 
> > If there are multiple interrupts, you best make them named interrupts,
> > i.e. requiring "interrupt-names", too.
> 
> Thanks, I will look into that.
> 
> > Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24.
> 
> Thanks for bringing that up. I think it has also come up elsewhere in this
> thread. I'd like to focus on addressing it here rather than spreading the
> discussion around any further.
> 
> My understanding is that on the R-Car Gen3 r8a7795 SoC
> the EthernetAVB hardware may function in one of two modes.
> 
> 1. A mode which is "mostly" compatible with R-Car Gen2
>    (e.g. r8a7790, r8a7790).
> 
>    In this mode only ch22 and ch24 are used.
> 
>    I note that the emac interrupt also appears to be documented for Gen-2.
>    Its not clear to me at this time why it isn't appropriate to use it
>    on those SoCs.

I have confirmed with that the emac interrupt shouldn't be used
on Gen2 SoCs.

> 
> 2. A mode which is new in Gen 3.
> 
>    In this mode ch0 - ch24 are used.
>    I believe that in this mode there are per DMA queue interrupts.


With regards to named interrupts, and indeed most of the other feedback I
have received for this patch, how about the following?

I also confirmed with Mizuguchi-san that it is intentional that the same
handler is used for both interrupts.


From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Subject: [PATCH/RFC v1.1] ravb: Add support for r8a7795 SoC

This patch supports the r8a7795 SoC by:
- Adding a compat string for the new hardware
- Support named-interrupts
- Support the E-DMAC interrupt on the new hardware.
  Although also present on Gen2 SoCs my understanding is
  that it can't be used on those SoCs.

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

---

v0 [Kazuya Mizuguchi]

v1 [Simon Horman]
* Updated patch subject

v1.1 [Simon Horman]
* As suggested by Sergei Shtylyov
  - Updated changelog
  - Corrected capitalisation in documentation
  - Removed pdev variable from ravb_open()
  - Corrected indentation
  - Leave initialisation and freeing of existing interrupt in common code
* As suggested by Geert Uytterhoeven and Sergei Shtylyov
  - support named interrupts

TODO:
* Consider propagating error for both new and existing call to
  platform_get_irq().
---
 .../devicetree/bindings/net/renesas,ravb.txt       | 12 ++++++-
 drivers/net/ethernet/renesas/ravb.h                |  1 +
 drivers/net/ethernet/renesas/ravb_main.c           | 38 +++++++++++++++++++---
 3 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
index 1fd8831437bf..e2b2a551813a 100644
--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
@@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
+	      these interrupts must be named.
+              One named or unnamed data 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 +22,12 @@ Required properties:
 Optional properties:
 - interrupt-parent: the phandle for the interrupt controller that services
 		    interrupts for this device.
+- interrupt-names: Names of named interrupts.
+                   If the property is present "data" is required.
+                   "emac" is also required for the R8A7795 SoC;
+		   it is prohibited otherwise.
+		   This property is mandatory for the R8A7795 SoC;
+		   optional otherwise.
 - pinctrl-names: pin configuration state name ("default").
 - renesas,no-ether-link: boolean, specify when a board does not provide a proper
 			 AVB_LINK signal.
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a157aaaaff6a..1832737063f3 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -809,6 +809,7 @@ struct ravb_private {
 
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
+	int emac_irq;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 026d98435d87..0276707089a5 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1185,6 +1185,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
 static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct device *dev = &priv->pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
@@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev)
 		goto out_napi_off;
 	}
 
+	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) {
+		error = request_irq(priv->emac_irq, ravb_interrupt,
+				    IRQF_SHARED, ndev->name, ndev);
+		if (error) {
+			netdev_err(ndev, "cannot request IRQ\n");
+			free_irq(ndev->irq, ndev);
+			goto out_napi_off;
+		}
+	}
+
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
@@ -1220,6 +1231,8 @@ out_ptp_stop:
 	ravb_ptp_stop(ndev);
 out_free_irq:
 	free_irq(ndev->irq, ndev);
+	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795"))
+		free_irq(priv->emac_irq, ndev);
 out_napi_off:
 	napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -1628,9 +1641,9 @@ static int ravb_mdio_release(struct ravb_private *priv)
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	int error, irq, q, is_r8a7795;
 	struct ravb_private *priv;
 	struct net_device *ndev;
-	int error, irq, q;
 	struct resource *res;
 
 	if (!np) {
@@ -1657,10 +1670,17 @@ 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);
+
+	is_r8a7795 = of_device_is_compatible(np, "renesas,etheravb-r8a7795");
+
+	irq = platform_get_irq_byname(pdev, "data");
 	if (irq < 0) {
-		error = -ENODEV;
-		goto out_release;
+		if (!is_r8a7795)
+			irq = platform_get_irq(pdev, 0);
+		if (irq < 0) {
+			error = -ENODEV;
+			goto out_release;
+		}
 	}
 	ndev->irq = irq;
 
@@ -1688,6 +1708,15 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low  		of_property_read_bool(np, "renesas,ether-link-active-low");
 
+	if (is_r8a7795) {
+		irq = platform_get_irq_byname(pdev, "emac");
+		if (irq < 0) {
+			error = -ENODEV;
+			goto out_release;
+		}
+		priv->emac_irq = irq;
+	}
+
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
@@ -1821,6 +1850,7 @@ static const struct dev_pm_ops ravb_dev_pm_ops = {
 static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790" },
 	{ .compatible = "renesas,etheravb-r8a7794" },
+	{ .compatible = "renesas,etheravb-r8a7795" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
-- 
2.1.4

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Geert Uytterhoeven]

Hi Simon,

Thanks for the update!

On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
> +             these interrupts must be named.
> +              One named or unnamed data 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 +22,12 @@ Required properties:
>  Optional properties:
>  - interrupt-parent: the phandle for the interrupt controller that services
>                     interrupts for this device.
> +- interrupt-names: Names of named interrupts.
> +                   If the property is present "data" is required.
> +                   "emac" is also required for the R8A7795 SoC;
> +                  it is prohibited otherwise.
> +                  This property is mandatory for the R8A7795 SoC;
> +                  optional otherwise.
>  - pinctrl-names: pin configuration state name ("default").
>  - renesas,no-ether-link: boolean, specify when a board does not provide a proper
>                          AVB_LINK signal.

What about the 25 channel interrupts?
"data" and "emac" seem to use ch22 resp. ch 24 on Gen3.

I'm afraid this will bite us one day.

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 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 10:35:56AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> Thanks for the update!
> 
> On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
> > +             these interrupts must be named.
> > +              One named or unnamed data 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 +22,12 @@ Required properties:
> >  Optional properties:
> >  - interrupt-parent: the phandle for the interrupt controller that services
> >                     interrupts for this device.
> > +- interrupt-names: Names of named interrupts.
> > +                   If the property is present "data" is required.
> > +                   "emac" is also required for the R8A7795 SoC;
> > +                  it is prohibited otherwise.
> > +                  This property is mandatory for the R8A7795 SoC;
> > +                  optional otherwise.
> >  - pinctrl-names: pin configuration state name ("default").
> >  - renesas,no-ether-link: boolean, specify when a board does not provide a proper
> >                          AVB_LINK signal.
> 
> What about the 25 channel interrupts?
> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
> 
> I'm afraid this will bite us one day.

Yes, I worry about that too. But perhaps we can disallow the use of
"data" and "emac" if all (any of) the chXX interrupts are named.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 8/28/2015 5:01 AM, Simon Horman wrote:

>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>
>>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>> [horms: updated changelog]
>>
>>     I don't see any. ;-)
>>
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>> ---
>>>   .../devicetree/bindings/net/renesas,ravb.txt       |  6 ++-
>>>   drivers/net/ethernet/renesas/ravb.h                |  1 +
>>>   drivers/net/ethernet/renesas/ravb_main.c           | 47 +++++++++++++++++++---
>>>   3 files changed, 47 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> index 1fd8831437bf..0b4ec02c35a4 100644
>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index a157aaaaff6a..1832737063f3 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -809,6 +809,7 @@ struct ravb_private {
>>>
>>>   	unsigned no_avb_link:1;
>>>   	unsigned avb_link_active_low:1;
>>> +	int emac_irq;
>>>   };

>>     Why not add it above the bit fields?

> Using an int seems reasonable to me as it is used to hold
> an integer value returned by request_irq().

    No question about the field type, only about its placement.

[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 026d98435d87..bf604a869458 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = {
[...]
>>> +		if (error) {
>>> +			netdev_err(ndev, "cannot request IRQ\n");
>>> +			goto out_napi_off;
>>> +		}
>>> +		error = request_irq(priv->emac_irq,
>>> +				ravb_interrupt,	IRQF_SHARED, ndev->name, ndev);
>>
>>     Likewise.
>>     And using the same handler for both interrupts doesn't look good.

> It does seem a little odd. I will confirm that detail.

    If the EMAC indeed uses a separate interrupt, please specify its handler 
directly. The same for the separate AVB-DMAC interrupt (I somewhat doubt that 
we really need it in the current driver though).

[...]

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

Hello.

On 8/28/2015 5:13 AM, Simon Horman wrote:

>>>> @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
>>>>   	priv->avb_link_active_low >>>>   		of_property_read_bool(np, "renesas,ether-link-active-low");
>>>>
>>>> +	if (of_device_is_compatible(np,
>>>> +				"renesas,etheravb-r8a7795")) {
>>>
>>>     Please respect the rules. And do you really need the break the line here?
>>>
>>>> +		irq = platform_get_irq(pdev, 1);
>>>> +		if (irq < 0) {
>>>> +			error = -ENODEV;
>>>
>>>     Please propagate the error instead.

>> Sure, will do.

> On closer examination the above is consistent with the existing
> error handling for another call to platform_get_irq() in the same function.
> Shall we fix that first?

    Yes, and I'll look into that.

WBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote:

> Thanks for the update!

> On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;

    Data?! What the heck does it mean? :-/

>> +             these interrupts must be named.
>> +              One named or unnamed data 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 +22,12 @@ Required properties:
>>   Optional properties:
>>   - interrupt-parent: the phandle for the interrupt controller that services
>>                      interrupts for this device.
>> +- interrupt-names: Names of named interrupts.
>> +                   If the property is present "data" is required.
>> +                   "emac" is also required for the R8A7795 SoC;
>> +                  it is prohibited otherwise.
>> +                  This property is mandatory for the R8A7795 SoC;
>> +                  optional otherwise.
>>   - pinctrl-names: pin configuration state name ("default").
>>   - renesas,no-ether-link: boolean, specify when a board does not provide a proper
>>                           AVB_LINK signal.
>
> What about the 25 channel interrupts?
> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
>
> I'm afraid this will bite us one day.

    Me too. We should describe the real hardware, not how the driver uses it. 
Where does configuring the AVB interrupt mode happen?

> Gr{oetje,eeting}s,
>                          Geert

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 8/28/2015 11:27 AM, Simon Horman wrote:

>>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>> @@ -6,8 +6,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
>>>> +              interrupt specifier for the sole interrupt.
>>>> +              if the device is a part of R8A7795 SoC
>>>> +              interrupt specifier for the two interrupts.
>>>
>>> If there are multiple interrupts, you best make them named interrupts,
>>> i.e. requiring "interrupt-names", too.
>>
>> Thanks, I will look into that.
>>
>>> Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24.
>>
>> Thanks for bringing that up. I think it has also come up elsewhere in this
>> thread. I'd like to focus on addressing it here rather than spreading the
>> discussion around any further.
>>
>> My understanding is that on the R-Car Gen3 r8a7795 SoC
>> the EthernetAVB hardware may function in one of two modes.
>>
>> 1. A mode which is "mostly" compatible with R-Car Gen2
>>     (e.g. r8a7790, r8a7790).
>>
>>     In this mode only ch22 and ch24 are used.

    But this is *not* really compatible!

>>     I note that the emac interrupt also appears to be documented for Gen-2.

    Where? I'm looking at the common h/w manual rev1.02 and only seeing IRQ 
162 for the normal Ether and IRQ163 for the EtherAVB...

>>     Its not clear to me at this time why it isn't appropriate to use it
>>     on those SoCs.

    I just never saw it in the manual.

> I have confirmed with that the emac interrupt shouldn't be used
> on Gen2 SoCs.

    Hm... It seems to me from re-reading the driver code, that the current 
driver only uses the *E-MAC* interrupts and doesn't use the AVB-DMAC 
interrupts. Maybe I'm over-simplifying though...

>> 2. A mode which is new in Gen 3.
>>
>>     In this mode ch0 - ch24 are used.
>>     I believe that in this mode there are per DMA queue interrupts.
 >
> With regards to named interrupts, and indeed most of the other feedback I
> have received for this patch, how about the following?
 >
> I also confirmed with Mizuguchi-san that it is intentional that the same
> handler is used for both interrupts.

    I don't doubt it was intentional, I just very much doubt that it's correct.

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Subject: [PATCH/RFC v1.1] ravb: Add support for r8a7795 SoC
>
> This patch supports the r8a7795 SoC by:
> - Adding a compat string for the new hardware
> - Support named-interrupts
> - Support the E-DMAC interrupt on the new hardware.
>    Although also present on Gen2 SoCs my understanding is
>    that it can't be used on those SoCs.

    No, I don't agree with this description, I don't know where you got this info.

> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> [horms: updated changelog and cleaned up]
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
>
> v0 [Kazuya Mizuguchi]
>
> v1 [Simon Horman]
> * Updated patch subject
>
> v1.1 [Simon Horman]
> * As suggested by Sergei Shtylyov
>    - Updated changelog
>    - Corrected capitalisation in documentation
>    - Removed pdev variable from ravb_open()
>    - Corrected indentation
>    - Leave initialisation and freeing of existing interrupt in common code
> * As suggested by Geert Uytterhoeven and Sergei Shtylyov
>    - support named interrupts
>
> TODO:
> * Consider propagating error for both new and existing call to
>    platform_get_irq().
> ---
>   .../devicetree/bindings/net/renesas,ravb.txt       | 12 ++++++-
>   drivers/net/ethernet/renesas/ravb.h                |  1 +
>   drivers/net/ethernet/renesas/ravb_main.c           | 38 +++++++++++++++++++---
>   3 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index 1fd8831437bf..e2b2a551813a 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;

    No, the "data" doesn't seem a correct name...

> +	      these interrupts must be named.
> +              One named or unnamed data 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.
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index a157aaaaff6a..1832737063f3 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -809,6 +809,7 @@ struct ravb_private {
>
>   	unsigned no_avb_link:1;
>   	unsigned avb_link_active_low:1;
> +	int emac_irq;

    Please place this somewhere before the bit fields.

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 026d98435d87..0276707089a5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev)
>   		goto out_napi_off;
>   	}
>
> +	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) {
> +		error = request_irq(priv->emac_irq, ravb_interrupt,

    No, I can't agree to that. You need to clearly separate the interrupt 
handlers.

> +				    IRQF_SHARED, ndev->name, ndev);
> +		if (error) {
> +			netdev_err(ndev, "cannot request IRQ\n");
> +			free_irq(ndev->irq, ndev);

    No, please just add a new label below instead, we already have free_irq() 
call there.

[...]
> @@ -1220,6 +1231,8 @@ out_ptp_stop:
>   	ravb_ptp_stop(ndev);
>   out_free_irq:
>   	free_irq(ndev->irq, ndev);
> +	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795"))

    Repeating this check all over again doesn't look good to me... can you see 
about adding some data to the 'struct of_device_id' initalizers?

[...]

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 01:20:49PM +0300, Sergei Shtylyov wrote:
> On 8/28/2015 5:01 AM, Simon Horman wrote:
> 
> >>>From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>
> >>>Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>[horms: updated changelog]
> >>
> >>    I don't see any. ;-)
> >>
> >>>Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >>>---
> >>>  .../devicetree/bindings/net/renesas,ravb.txt       |  6 ++-
> >>>  drivers/net/ethernet/renesas/ravb.h                |  1 +
> >>>  drivers/net/ethernet/renesas/ravb_main.c           | 47 +++++++++++++++++++---
> >>>  3 files changed, 47 insertions(+), 7 deletions(-)
> >>>
> >>>diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>>index 1fd8831437bf..0b4ec02c35a4 100644
> >>>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> [...]
> >>>diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >>>index a157aaaaff6a..1832737063f3 100644
> >>>--- a/drivers/net/ethernet/renesas/ravb.h
> >>>+++ b/drivers/net/ethernet/renesas/ravb.h
> >>>@@ -809,6 +809,7 @@ struct ravb_private {
> >>>
> >>>  	unsigned no_avb_link:1;
> >>>  	unsigned avb_link_active_low:1;
> >>>+	int emac_irq;
> >>>  };
> 
> >>    Why not add it above the bit fields?
> 
> >Using an int seems reasonable to me as it is used to hold
> >an integer value returned by request_irq().
> 
>    No question about the field type, only about its placement.

Sorry, I miss read your comment.
I'll move the field if you like.

> [...]
> >>>diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>>index 026d98435d87..bf604a869458 100644
> >>>--- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>@@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = {
> [...]
> >>>+		if (error) {
> >>>+			netdev_err(ndev, "cannot request IRQ\n");
> >>>+			goto out_napi_off;
> >>>+		}
> >>>+		error = request_irq(priv->emac_irq,
> >>>+				ravb_interrupt,	IRQF_SHARED, ndev->name, ndev);
> >>
> >>    Likewise.
> >>    And using the same handler for both interrupts doesn't look good.
> 
> >It does seem a little odd. I will confirm that detail.
> 
>    If the EMAC indeed uses a separate interrupt, please specify its handler
> directly. The same for the separate AVB-DMAC interrupt (I somewhat doubt
> that we really need it in the current driver though).

Ok, I will look into making it so.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 08/28/2015 01:51 PM, Sergei Shtylyov wrote:

>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
>
>     Data?! What the heck does it mean? :-/

    Oh, and the manual consistently spells "emac" as E-MAC.

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

On 08/28/2015 01:30 PM, Sergei Shtylyov wrote:

>>>>> @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
>>>>>       priv->avb_link_active_low >>>>>           of_property_read_bool(np, "renesas,ether-link-active-low");
>>>>>
>>>>> +    if (of_device_is_compatible(np,
>>>>> +                "renesas,etheravb-r8a7795")) {
>>>>
>>>>     Please respect the rules. And do you really need the break the line here?
>>>>
>>>>> +        irq = platform_get_irq(pdev, 1);
>>>>> +        if (irq < 0) {
>>>>> +            error = -ENODEV;
>>>>
>>>>     Please propagate the error instead.
>
>>> Sure, will do.
>
>> On closer examination the above is consistent with the existing
>> error handling for another call to platform_get_irq() in the same function.
>> Shall we fix that first?

>     Yes, and I'll look into that.

    Accidentally spotted the same error in sh_eth.c... really old one. :-) 
Will fix both.

WBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 02:46:30PM +0300, Sergei Shtylyov wrote:
> On 8/28/2015 11:27 AM, Simon Horman wrote:
> 
> >>>>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>>>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>>>@@ -6,8 +6,12 @@ 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: if the device is a part of R8A7790/R8A7794 SoC
> >>>>+              interrupt specifier for the sole interrupt.
> >>>>+              if the device is a part of R8A7795 SoC
> >>>>+              interrupt specifier for the two interrupts.
> >>>
> >>>If there are multiple interrupts, you best make them named interrupts,
> >>>i.e. requiring "interrupt-names", too.
> >>
> >>Thanks, I will look into that.
> >>
> >>>Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24.
> >>
> >>Thanks for bringing that up. I think it has also come up elsewhere in this
> >>thread. I'd like to focus on addressing it here rather than spreading the
> >>discussion around any further.
> >>
> >>My understanding is that on the R-Car Gen3 r8a7795 SoC
> >>the EthernetAVB hardware may function in one of two modes.
> >>
> >>1. A mode which is "mostly" compatible with R-Car Gen2
> >>    (e.g. r8a7790, r8a7790).
> >>
> >>    In this mode only ch22 and ch24 are used.
> 
>    But this is *not* really compatible!

Compatible may not have been the wisest choice of wording.
Regardless, the point is that there are two modes. I will describe
them more fully in a separate sub-thread.

> >>    I note that the emac interrupt also appears to be documented for Gen-2.
> 
>    Where? I'm looking at the common h/w manual rev1.02 and only seeing IRQ
> 162 for the normal Ether and IRQ163 for the EtherAVB...
> 
> >>    Its not clear to me at this time why it isn't appropriate to use it
> >>    on those SoCs.
> 
>    I just never saw it in the manual.

Sorry, I was mistaken.
On closer inspection I don't see it either.

> >I have confirmed with that the emac interrupt shouldn't be used
> >on Gen2 SoCs.
> 
>    Hm... It seems to me from re-reading the driver code, that the current
> driver only uses the *E-MAC* interrupts and doesn't use the AVB-DMAC
> interrupts. Maybe I'm over-simplifying though...

I'm not sure that I understand the distinction that you are making there.

> >>2. A mode which is new in Gen 3.
> >>
> >>    In this mode ch0 - ch24 are used.
> >>    I believe that in this mode there are per DMA queue interrupts.
> >
> >With regards to named interrupts, and indeed most of the other feedback I
> >have received for this patch, how about the following?
> >
> >I also confirmed with Mizuguchi-san that it is intentional that the same
> >handler is used for both interrupts.
> 
>    I don't doubt it was intentional, I just very much doubt that it's correct.

As mentioned above, I have will describe more fully the interrupts in
another sub-thread.  I hope that helps clear things up.

My position on using a shared handler is that it is a simple step
to create a correct implementation. I believe it is correct because
the existing handler handles a superset of the interrupts from each of the
two lines hooked up to it will generate.

I believe that it in the longer term it will make for a nicer
implementation to have specific handlers for each interrupt.
But it that it makes sense to handle that as incremental updates
as the driver matures.

> 
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >Subject: [PATCH/RFC v1.1] ravb: Add support for r8a7795 SoC
> >
> >This patch supports the r8a7795 SoC by:
> >- Adding a compat string for the new hardware
> >- Support named-interrupts
> >- Support the E-DMAC interrupt on the new hardware.
> >   Although also present on Gen2 SoCs my understanding is
> >   that it can't be used on those SoCs.
> 
>    No, I don't agree with this description, I don't know where you got this info.

Thanks, I withdraw that description.

> >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >[horms: updated changelog and cleaned up]
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> >---
> >
> >v0 [Kazuya Mizuguchi]
> >
> >v1 [Simon Horman]
> >* Updated patch subject
> >
> >v1.1 [Simon Horman]
> >* As suggested by Sergei Shtylyov
> >   - Updated changelog
> >   - Corrected capitalisation in documentation
> >   - Removed pdev variable from ravb_open()
> >   - Corrected indentation
> >   - Leave initialisation and freeing of existing interrupt in common code
> >* As suggested by Geert Uytterhoeven and Sergei Shtylyov
> >   - support named interrupts
> >
> >TODO:
> >* Consider propagating error for both new and existing call to
> >   platform_get_irq().
> >---
> >  .../devicetree/bindings/net/renesas,ravb.txt       | 12 ++++++-
> >  drivers/net/ethernet/renesas/ravb.h                |  1 +
> >  drivers/net/ethernet/renesas/ravb_main.c           | 38 +++++++++++++++++++---
> >  3 files changed, 46 insertions(+), 5 deletions(-)
> >
> >diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >index 1fd8831437bf..e2b2a551813a 100644
> >--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >@@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
> 
>    No, the "data" doesn't seem a correct name...

I will respond more fully  with regards to the bindings in another
sub-thread.

> >+	      these interrupts must be named.
> >+              One named or unnamed data 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.
> [...]
> >diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >index a157aaaaff6a..1832737063f3 100644
> >--- a/drivers/net/ethernet/renesas/ravb.h
> >+++ b/drivers/net/ethernet/renesas/ravb.h
> >@@ -809,6 +809,7 @@ struct ravb_private {
> >
> >  	unsigned no_avb_link:1;
> >  	unsigned avb_link_active_low:1;
> >+	int emac_irq;
> 
>    Please place this somewhere before the bit fields.

Will do.

> [...]
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index 026d98435d87..0276707089a5 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> >@@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev)
> >  		goto out_napi_off;
> >  	}
> >
> >+	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) {
> >+		error = request_irq(priv->emac_irq, ravb_interrupt,
> 
>    No, I can't agree to that. You need to clearly separate the interrupt
> handlers.
> 
> >+				    IRQF_SHARED, ndev->name, ndev);
> >+		if (error) {
> >+			netdev_err(ndev, "cannot request IRQ\n");
> >+			free_irq(ndev->irq, ndev);
> 
>    No, please just add a new label below instead, we already have free_irq()
> call there.

Sure, will do.

> [...]
> >@@ -1220,6 +1231,8 @@ out_ptp_stop:
> >  	ravb_ptp_stop(ndev);
> >  out_free_irq:
> >  	free_irq(ndev->irq, ndev);
> >+	if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795"))
> 
>    Repeating this check all over again doesn't look good to me... can you
> see about adding some data to the 'struct of_device_id' initalizers?

Good idea, I'll look into it.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 01:51:20PM +0300, Sergei Shtylyov wrote:
> On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote:
> 
> >Thanks for the update!
> 
> >On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
> >>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> >>@@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
> 
>    Data?! What the heck does it mean? :-/

Perhaps it would be better to refer to them as dmac interrupts.

The documentation makes reference to "merged data interrupt", which
seems to refer to tx and rx interrupts. But this interrupt is also
used for error and management related interrupts.

I'm all ears with regards to a good name.

With regards to the documentation consistently referring to "E-MAC",
which you raised elsewhere. Yes I see that too but I'm not sure where
you are going there. If you would like to tweak the documentation
or name of the interrupt somehow please spell out your ideas a little
more clearly.

> >>+             these interrupts must be named.
> >>+              One named or unnamed data 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 +22,12 @@ Required properties:
> >>  Optional properties:
> >>  - interrupt-parent: the phandle for the interrupt controller that services
> >>                     interrupts for this device.
> >>+- interrupt-names: Names of named interrupts.
> >>+                   If the property is present "data" is required.
> >>+                   "emac" is also required for the R8A7795 SoC;
> >>+                  it is prohibited otherwise.
> >>+                  This property is mandatory for the R8A7795 SoC;
> >>+                  optional otherwise.
> >>  - pinctrl-names: pin configuration state name ("default").
> >>  - renesas,no-ether-link: boolean, specify when a board does not provide a proper
> >>                          AVB_LINK signal.
> >
> >What about the 25 channel interrupts?
> >"data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
> >
> >I'm afraid this will bite us one day.
> 
>    Me too. We should describe the real hardware, not how the driver uses it.
> Where does configuring the AVB interrupt mode happen?

I believe that we all want that. Lets see about making it so :)

As I mentioned above, broadly speaking the interrupts may be configured in
one of two modes. And the default configuration is to use what I earlier
described as "compatible". I will now just refer to it as mode 1 with
the other mode being mode 2.

Mode 1.

  ch22 or ch23 is used for:
   * Merged Data Interrupts (TX and RX)
   * Error related interrupts
   * Management related interrupts

  ch24 is used for:
   * E-MAC interrupts

  It seems to me that the above basically splits the single interrupt
  in Gen2 into two.

  This is the default mode for the hardware and the mode it ends up
  in using the current driver implementation (+ this series which doesn't
  alter the initialisation of the hardware).

Mode 2.

  ch0 - ch17 are used for:
  * per-queue Rx interrupts

  ch18 - ch21 are used for:
  * per-queue Tx interrupts

  ch22 or ch23 are used for:
  * Other DMA descriptor interrupts (if enabled)
  * Error related interrupts 
  * Management related interrupts

  ch24 is used for
  * E-MAC interrupts


I think it is instructive to examine differences between the modes for
Each interrupt. And here is how I see that:

  * ch0 - ch21:
    mode 1: unused
    mode 2: per queue rx or tx interrupts

  * ch22 or ch23:
    both mode 1 and 2:
    + Error related interrupts
    + Management related interrupts
    mode 1 only:
    + Merged Data Interrupts (TX and RX)
    mode 2 only:
    + Other DMA descriptor interrupts (if enabled)

  * ch24:
    mode 1 and 2: E-MAC interrupts


So it seems to me that one possibility would be to:
* Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac.
  - Better names could be found for data and dataB.
* Allow for properties to describe the mode and possibly which od dataA or B
  should be used.
But for now:
* Only describe the interrupts that are used and default to mode 1 with the
  data interrupt and;
* Defer defining properties to switch to combinations

I am all ears for other ideas.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

On Fri, Aug 28, 2015 at 04:21:14PM +0300, Sergei Shtylyov wrote:
> On 08/28/2015 01:30 PM, Sergei Shtylyov wrote:
> 
> >>>>>@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev)
> >>>>>      priv->avb_link_active_low > >>>>>          of_property_read_bool(np, "renesas,ether-link-active-low");
> >>>>>
> >>>>>+    if (of_device_is_compatible(np,
> >>>>>+                "renesas,etheravb-r8a7795")) {
> >>>>
> >>>>    Please respect the rules. And do you really need the break the line here?
> >>>>
> >>>>>+        irq = platform_get_irq(pdev, 1);
> >>>>>+        if (irq < 0) {
> >>>>>+            error = -ENODEV;
> >>>>
> >>>>    Please propagate the error instead.
> >
> >>>Sure, will do.
> >
> >>On closer examination the above is consistent with the existing
> >>error handling for another call to platform_get_irq() in the same function.
> >>Shall we fix that first?
> 
> >    Yes, and I'll look into that.
> 
>    Accidentally spotted the same error in sh_eth.c... really old one. :-)
> Will fix both.

Thanks, I see both of those changes went into net-next.
I'll update this patch accordingly.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Geert Uytterhoeven]

Hi Simon,

On Wed, Sep 2, 2015 at 4:13 AM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 28, 2015 at 01:51:20PM +0300, Sergei Shtylyov wrote:
>> On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote:
>> >On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
>> >>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>> >>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>> >>@@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
>>
>>    Data?! What the heck does it mean? :-/
>
> Perhaps it would be better to refer to them as dmac interrupts.
>
> The documentation makes reference to "merged data interrupt", which
> seems to refer to tx and rx interrupts. But this interrupt is also
> used for error and management related interrupts.
>
> I'm all ears with regards to a good name.
>
> With regards to the documentation consistently referring to "E-MAC",
> which you raised elsewhere. Yes I see that too but I'm not sure where
> you are going there. If you would like to tweak the documentation
> or name of the interrupt somehow please spell out your ideas a little
> more clearly.
>
>> >>+             these interrupts must be named.
>> >>+              One named or unnamed data 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 +22,12 @@ Required properties:
>> >>  Optional properties:
>> >>  - interrupt-parent: the phandle for the interrupt controller that services
>> >>                     interrupts for this device.
>> >>+- interrupt-names: Names of named interrupts.
>> >>+                   If the property is present "data" is required.
>> >>+                   "emac" is also required for the R8A7795 SoC;
>> >>+                  it is prohibited otherwise.
>> >>+                  This property is mandatory for the R8A7795 SoC;
>> >>+                  optional otherwise.
>> >>  - pinctrl-names: pin configuration state name ("default").
>> >>  - renesas,no-ether-link: boolean, specify when a board does not provide a proper
>> >>                          AVB_LINK signal.
>> >
>> >What about the 25 channel interrupts?
>> >"data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
>> >
>> >I'm afraid this will bite us one day.
>>
>>    Me too. We should describe the real hardware, not how the driver uses it.
>> Where does configuring the AVB interrupt mode happen?
>
> I believe that we all want that. Lets see about making it so :)
>
> As I mentioned above, broadly speaking the interrupts may be configured in
> one of two modes. And the default configuration is to use what I earlier
> described as "compatible". I will now just refer to it as mode 1 with
> the other mode being mode 2.
>
> Mode 1.
>
>   ch22 or ch23 is used for:
>    * Merged Data Interrupts (TX and RX)
>    * Error related interrupts
>    * Management related interrupts
>
>   ch24 is used for:
>    * E-MAC interrupts
>
>   It seems to me that the above basically splits the single interrupt
>   in Gen2 into two.
>
>   This is the default mode for the hardware and the mode it ends up
>   in using the current driver implementation (+ this series which doesn't
>   alter the initialisation of the hardware).
>
> Mode 2.
>
>   ch0 - ch17 are used for:
>   * per-queue Rx interrupts
>
>   ch18 - ch21 are used for:
>   * per-queue Tx interrupts
>
>   ch22 or ch23 are used for:
>   * Other DMA descriptor interrupts (if enabled)
>   * Error related interrupts
>   * Management related interrupts
>
>   ch24 is used for
>   * E-MAC interrupts
>
>
> I think it is instructive to examine differences between the modes for
> Each interrupt. And here is how I see that:
>
>   * ch0 - ch21:
>     mode 1: unused
>     mode 2: per queue rx or tx interrupts
>
>   * ch22 or ch23:
>     both mode 1 and 2:
>     + Error related interrupts
>     + Management related interrupts
>     mode 1 only:
>     + Merged Data Interrupts (TX and RX)
>     mode 2 only:
>     + Other DMA descriptor interrupts (if enabled)
>
>   * ch24:
>     mode 1 and 2: E-MAC interrupts

Thanks for the analysis and the explanation!

> So it seems to me that one possibility would be to:
> * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac.
>   - Better names could be found for data and dataB.

That's one possibility, but it looks to me like the rx* and tx* naming would
be purely a configuration, which doesn't describe the hardware, and perhaps
could even change for future parts? At least having 18 RX channels and 4
TX channels looks arbitrary to me.

> * Allow for properties to describe the mode and possibly which od dataA or B
>   should be used.

Also configuration, not description of the hardware.

> But for now:
> * Only describe the interrupts that are used and default to mode 1 with the
>   data interrupt and;
> * Defer defining properties to switch to combinations
>
> I am all ears for other ideas.

I think there should be either a single multiplexed interrupt (for Gen2),
or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are
mandatory (for Gen3). Other SoCs may reuse the RAVB IP core without
wiring up all channels.

For Gen3, the driver can then choose itself between mode1 and mode2
(if implemented in the driver, and all needed channels are available).

Does this sound sane?

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 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

Hi Geert,

On Wed, Sep 02, 2015 at 09:26:32AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,

[snip]

> Thanks for the analysis and the explanation!
> 
> > So it seems to me that one possibility would be to:
> > * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac.
> >   - Better names could be found for data and dataB.
> 
> That's one possibility, but it looks to me like the rx* and tx* naming would
> be purely a configuration, which doesn't describe the hardware, and perhaps
> could even change for future parts? At least having 18 RX channels and 4
> TX channels looks arbitrary to me.
> 
> > * Allow for properties to describe the mode and possibly which od dataA or B
> >   should be used.
> 
> Also configuration, not description of the hardware.
> 
> > But for now:
> > * Only describe the interrupts that are used and default to mode 1 with the
> >   data interrupt and;
> > * Defer defining properties to switch to combinations
> >
> > I am all ears for other ideas.
> 
> I think there should be either a single multiplexed interrupt (for Gen2),
> or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are
> mandatory (for Gen3). Other SoCs may reuse the RAVB IP core without
> wiring up all channels.
> 
> For Gen3, the driver can then choose itself between mode1 and mode2
> (if implemented in the driver, and all needed channels are available).
> 
> Does this sound sane?

Sure, that sounds sane to me.

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

Hello.

On 09/02/2015 10:26 AM, Geert Uytterhoeven wrote:

>>>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>>> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
>>>
>>>     Data?! What the heck does it mean? :-/
>>
>> Perhaps it would be better to refer to them as dmac interrupts.
>>
>> The documentation makes reference to "merged data interrupt", which
>> seems to refer to tx and rx interrupts. But this interrupt is also
>> used for error and management related interrupts.
>>
>> I'm all ears with regards to a good name.
>>
>> With regards to the documentation consistently referring to "E-MAC",
>> which you raised elsewhere. Yes I see that too but I'm not sure where
>> you are going there. If you would like to tweak the documentation
>> or name of the interrupt somehow please spell out your ideas a little
>> more clearly.
>>
>>>>> +             these interrupts must be named.
>>>>> +              One named or unnamed data 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 +22,12 @@ Required properties:
>>>>>   Optional properties:
>>>>>   - interrupt-parent: the phandle for the interrupt controller that services
>>>>>                      interrupts for this device.
>>>>> +- interrupt-names: Names of named interrupts.
>>>>> +                   If the property is present "data" is required.
>>>>> +                   "emac" is also required for the R8A7795 SoC;
>>>>> +                  it is prohibited otherwise.
>>>>> +                  This property is mandatory for the R8A7795 SoC;
>>>>> +                  optional otherwise.
>>>>>   - pinctrl-names: pin configuration state name ("default").
>>>>>   - renesas,no-ether-link: boolean, specify when a board does not provide a proper
>>>>>                           AVB_LINK signal.
>>>>
>>>> What about the 25 channel interrupts?
>>>> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
>>>>
>>>> I'm afraid this will bite us one day.
>>>
>>>     Me too. We should describe the real hardware, not how the driver uses it.
>>> Where does configuring the AVB interrupt mode happen?

    Answering my own question: it's done by the new registers of the EtherAVB 
controller. I now have the gen3 manual.

>> I believe that we all want that. Lets see about making it so :)
>>
>> As I mentioned above, broadly speaking the interrupts may be configured in
>> one of two modes. And the default configuration is to use what I earlier
>> described as "compatible". I will now just refer to it as mode 1 with
>> the other mode being mode 2.
>>
>> Mode 1.
>>
>>    ch22 or ch23 is used for:
>>     * Merged Data Interrupts (TX and RX)

    No, only ch22 can be used fo those.

>>     * Error related interrupts
>>     * Management related interrupts

>>    ch24 is used for:
>>     * E-MAC interrupts

>>    It seems to me that the above basically splits the single interrupt
>>    in Gen2 into two.
>>
>>    This is the default mode for the hardware and the mode it ends up
>>    in using the current driver implementation (+ this series which doesn't
>>    alter the initialisation of the hardware).

    This is also assuming that the bootloader doesn't change it. Are we sure 
it doesn't?

>> Mode 2.
>>
>>    ch0 - ch17 are used for:
>>    * per-queue Rx interrupts
>>
>>    ch18 - ch21 are used for:
>>    * per-queue Tx interrupts
>>
>>    ch22 or ch23 are used for:
>>    * Other DMA descriptor interrupts (if enabled)
>>    * Error related interrupts
>>    * Management related interrupts
>>
>>    ch24 is used for
>>    * E-MAC interrupts
>>
>>
>> I think it is instructive to examine differences between the modes for
>> Each interrupt. And here is how I see that:
>>
>>    * ch0 - ch21:
>>      mode 1: unused
>>      mode 2: per queue rx or tx interrupts
>>
>>    * ch22 or ch23:
>>      both mode 1 and 2:
>>      + Error related interrupts
>>      + Management related interrupts
>>      mode 1 only:
>>      + Merged Data Interrupts (TX and RX)
>>      mode 2 only:
>>      + Other DMA descriptor interrupts (if enabled)
>>
>>    * ch24:
>>      mode 1 and 2: E-MAC interrupts

> Thanks for the analysis and the explanation!

    Yes, although it doesn't quite agree with the manual I have (rev 0.5E).

>> So it seems to me that one possibility would be to:
>> * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac.
>>    - Better names could be found for data and dataB.

> That's one possibility, but it looks to me like the rx* and tx* naming would
> be purely a configuration, which doesn't describe the hardware,

    No, it does, according to the table 50.19 in the manual.

> and perhaps
> could even change for future parts? At least having 18 RX channels and 4
> TX channels looks arbitrary to me.

    These #s are the same across the known EtherAVB implementations.

>> * Allow for properties to describe the mode and possibly which od dataA or B
>>    should be used.

> Also configuration, not description of the hardware.

    Agreed here.

>> But for now:
>> * Only describe the interrupts that are used and default to mode 1 with the
>>    data interrupt and;
>> * Defer defining properties to switch to combinations
>>
>> I am all ears for other ideas.
>
> I think there should be either a single multiplexed interrupt (for Gen2),
> or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are
> mandatory (for Gen3).

    No, all IRQs should be mandatory. Otherwise we're into describing the 
config. again, not the real hardware.

> Other SoCs may reuse the RAVB IP core without
> wiring up all channels.

> For Gen3, the driver can then choose itself between mode1 and mode2
> (if implemented in the driver, and all needed channels are available).

    Agreed.

> Does this sound sane?

    Mostly. :-)

> Gr{oetje,eeting}s,
>                          Geert

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Sergei Shtylyov]

Hello.

On 09/02/2015 05:13 AM, Simon Horman wrote:

>>> Thanks for the update!
>>
>>> On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote:
>>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
>>>> @@ -6,8 +6,12 @@ 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 data and one emac interrupt for the R8A7795 SoC;
>>
>>     Data?! What the heck does it mean? :-/
>
> Perhaps it would be better to refer to them as dmac interrupts.

    No, they do not seem to be limited to AVB-DMAC.

> The documentation makes reference to "merged data interrupt", which
> seems to refer to tx and rx interrupts. But this interrupt is also
> used for error and management related interrupts.

    Yes.

> I'm all ears with regards to a good name.

    AFAICS from the manual, there are many reasons for these, perhaps calling 
them "a" for ch22 (and "b" for ch23) would make more sense.

> With regards to the documentation consistently referring to "E-MAC",
> which you raised elsewhere. Yes I see that too but I'm not sure where
> you are going there.  If you would like to tweak the documentation
> or name of the interrupt somehow please spell out your ideas a little
> more clearly.

    Please just don't call it emac in the prop description, call it E-MAC, 
that's all.

>>>> +             these interrupts must be named.
>>>> +              One named or unnamed data 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 +22,12 @@ Required properties:
>>>>   Optional properties:
>>>>   - interrupt-parent: the phandle for the interrupt controller that services
>>>>                      interrupts for this device.
>>>> +- interrupt-names: Names of named interrupts.
>>>> +                   If the property is present "data" is required.
>>>> +                   "emac" is also required for the R8A7795 SoC;
>>>> +                  it is prohibited otherwise.
>>>> +                  This property is mandatory for the R8A7795 SoC;
>>>> +                  optional otherwise.
>>>>   - pinctrl-names: pin configuration state name ("default").
>>>>   - renesas,no-ether-link: boolean, specify when a board does not provide a proper
>>>>                           AVB_LINK signal.
>>>
>>> What about the 25 channel interrupts?
>>> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3.
>>>
>>> I'm afraid this will bite us one day.
>>
>>     Me too. We should describe the real hardware, not how the driver uses it.
>> Where does configuring the AVB interrupt mode happen?

    Now I'm seeing it happens in the controller itself.

> I believe that we all want that. Lets see about making it so :)
>
> As I mentioned above, broadly speaking the interrupts may be configured in
> one of two modes.

    There are many bits controlling the interrupt routing, so not sure we're 
limited to just 2 modes.

> And the default configuration is to use what I earlier
> described as "compatible". I will now just refer to it as mode 1 with
> the other mode being mode 2.

    I believe I have replied to the text below in a mail to Geert.

[...]

MBR, Sergei

Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman]

Hi,

On Tue, Sep 08, 2015 at 02:06:17AM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/02/2015 10:26 AM, Geert Uytterhoeven wrote:

[snip]

> >>   It seems to me that the above basically splits the single interrupt
> >>   in Gen2 into two.
> >>
> >>   This is the default mode for the hardware and the mode it ends up
> >>   in using the current driver implementation (+ this series which doesn't
> >>   alter the initialisation of the hardware).
> 
>    This is also assuming that the bootloader doesn't change it. Are we sure
> it doesn't?

If it is important I can check with regards to the current uboot (plans).

From my point of view it is a reasonable assumption to make.

[snip]

> >>But for now:
> >>* Only describe the interrupts that are used and default to mode 1 with the
> >>   data interrupt and;
> >>* Defer defining properties to switch to combinations
> >>
> >>I am all ears for other ideas.
> >
> >I think there should be either a single multiplexed interrupt (for Gen2),
> >or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are
> >mandatory (for Gen3).
> 
>    No, all IRQs should be mandatory. Otherwise we're into describing the
> config. again, not the real hardware.

That is fine by me.

[snip]