[PATCH 0/3] Add CAN support to rcar_can driver

[PATCH 0/3] Add CAN support to rcar_can driver

[Fabrizio Castro]

Dear All,

RZ/G2 is slightly different from other R-Car and RZ/G1 SoCs as clkp2
isn't available on RZ/G2 devices. Changes to driver and documentation
are therefore necessary and taken care of here.

Thanks,
Fab

Fabrizio Castro (3):
  can: rcar_can: Fix erroneous registration
  can: rcar_can: Add RZ/G2 support
  dt-bindings: can: rcar_can: Add r8a774a1 support

 .../devicetree/bindings/net/can/rcar_can.txt       | 11 +++--
 drivers/net/can/rcar/rcar_can.c                    | 48 ++++++++++++++++++----
 2 files changed, 48 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/3][can-next] can: rcar_can: Fix erroneous registration

[Fabrizio Castro]

Assigning 2 to "renesas,can-clock-select" tricks the driver into
registering the CAN interface, even though we don't want that.
This patch fixes this problem and also allows for architectures
missing some of the clocks (e.g. RZ/G2) to behave as expected.

Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
---

This patch applies on linux-can-next-for-4.19-20180727

 drivers/net/can/rcar/rcar_can.c | 43 +++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 11662f4..fbd9284 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -21,9 +21,13 @@
 #include <linux/clk.h>
 #include <linux/can/platform/rcar_can.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 #define RCAR_CAN_DRV_NAME	"rcar_can"
 
+#define RCAR_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
+				 BIT(CLKR_CLKEXT))
+
 /* Mailbox configuration:
  * mailbox 60 - 63 - Rx FIFO mailboxes
  * mailbox 56 - 59 - Tx FIFO mailboxes
@@ -745,10 +749,12 @@ static int rcar_can_probe(struct platform_device *pdev)
 	u32 clock_select = CLKR_CLKP1;
 	int err = -ENODEV;
 	int irq;
+	uintptr_t allowed_clks = RCAR_SUPPORTED_CLOCKS;
 
 	if (pdev->dev.of_node) {
 		of_property_read_u32(pdev->dev.of_node,
 				     "renesas,can-clock-select", &clock_select);
+		allowed_clks = (uintptr_t)of_device_get_match_data(&pdev->dev);
 	} else {
 		pdata = dev_get_platdata(&pdev->dev);
 		if (!pdata) {
@@ -789,7 +795,7 @@ static int rcar_can_probe(struct platform_device *pdev)
 		goto fail_clk;
 	}
 
-	if (clock_select >= ARRAY_SIZE(clock_names)) {
+	if (!(BIT(clock_select) & allowed_clks)) {
 		err = -EINVAL;
 		dev_err(&pdev->dev, "invalid CAN clock selected\n");
 		goto fail_clk;
@@ -899,13 +905,34 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
 
 static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
-	{ .compatible = "renesas,can-r8a7778" },
-	{ .compatible = "renesas,can-r8a7779" },
-	{ .compatible = "renesas,can-r8a7790" },
-	{ .compatible = "renesas,can-r8a7791" },
-	{ .compatible = "renesas,rcar-gen1-can" },
-	{ .compatible = "renesas,rcar-gen2-can" },
-	{ .compatible = "renesas,rcar-gen3-can" },
+	{
+		.compatible = "renesas,can-r8a7778",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,can-r8a7779",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,can-r8a7790",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,can-r8a7791",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,rcar-gen1-can",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,rcar-gen2-can",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
+	{
+		.compatible = "renesas,rcar-gen3-can",
+		.data = (void *)RCAR_SUPPORTED_CLOCKS,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, rcar_can_of_table);
-- 
2.7.4

[PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support

[Fabrizio Castro]

RZ/G2 devices don't have clkp2, therefore this commit adds a
generic compatible string for them to allow for proper checking
during probe.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
---

This patch applies on linux-can-next-for-4.19-20180727

 drivers/net/can/rcar/rcar_can.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index fbd9284..397208e 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -27,6 +27,7 @@
 
 #define RCAR_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
 				 BIT(CLKR_CLKEXT))
+#define RZG2_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKEXT))
 
 /* Mailbox configuration:
  * mailbox 60 - 63 - Rx FIFO mailboxes
@@ -933,6 +934,10 @@ static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
 		.compatible = "renesas,rcar-gen3-can",
 		.data = (void *)RCAR_SUPPORTED_CLOCKS,
 	},
+	{
+		.compatible = "renesas,rzg-gen2-can",
+		.data = (void *)RZG2_SUPPORTED_CLOCKS,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, rcar_can_of_table);
-- 
2.7.4

[PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support

[Fabrizio Castro]

Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
generic bindings.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
Reviewed-by: Biju Das <biju.das@bp.renesas.com>
---

This patch applies on next-20180823

 Documentation/devicetree/bindings/net/can/rcar_can.txt | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
index 94a7f33..ae8fccc 100644
--- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
+++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
@@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
 Required properties:
 - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
 	      "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
+	      "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.
 	      "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
 	      "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
 	      "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
@@ -17,6 +18,7 @@ Required properties:
 	      "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
 	      compatible device.
 	      "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
+	      "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.
 	      When compatible with the generic version, nodes must list the
 	      SoC-specific version corresponding to the platform first
 	      followed by the generic version.
@@ -24,7 +26,9 @@ Required properties:
 - reg: physical base address and size of the R-Car CAN register map.
 - interrupts: interrupt specifier for the sole interrupt.
 - clocks: phandles and clock specifiers for 3 CAN clock inputs.
-- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
+- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
+	       3 clock input name strings for every other SoC: "clkp1", "clkp2",
+	       "can_clk".
 - pinctrl-0: pin control group to be used for this controller.
 - pinctrl-names: must be "default".
 
@@ -41,8 +45,9 @@ using the below properties:
 Optional properties:
 - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
 			    <0x0> (default) : Peripheral clock (clkp1)
-			    <0x1> : Peripheral clock (clkp2)
-			    <0x3> : Externally input clock
+			    <0x1> : Peripheral clock (clkp2) (not supported by
+				    RZ/G2 devices)
+			    <0x3> : External input clock
 
 Example
 -------
-- 
2.7.4

Re: [PATCH 1/3][can-next] can: rcar_can: Fix erroneous registration

[Simon Horman]

On Thu, Aug 23, 2018 at 02:07:31PM +0100, Fabrizio Castro wrote:
> Assigning 2 to "renesas,can-clock-select" tricks the driver into
> registering the CAN interface, even though we don't want that.
> This patch fixes this problem and also allows for architectures
> missing some of the clocks (e.g. RZ/G2) to behave as expected.
> 
> Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>

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

Re: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support

[Simon Horman]

On Thu, Aug 23, 2018 at 02:07:32PM +0100, Fabrizio Castro wrote:
> RZ/G2 devices don't have clkp2, therefore this commit adds a
> generic compatible string for them to allow for proper checking
> during probe.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>


Are we sure these clocks are for all RZ/G2 SoCs?

If so


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


> ---
> 
> This patch applies on linux-can-next-for-4.19-20180727
> 
>  drivers/net/can/rcar/rcar_can.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> index fbd9284..397208e 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -27,6 +27,7 @@
>  
>  #define RCAR_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
>  				 BIT(CLKR_CLKEXT))
> +#define RZG2_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKEXT))
>  
>  /* Mailbox configuration:
>   * mailbox 60 - 63 - Rx FIFO mailboxes
> @@ -933,6 +934,10 @@ static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
>  		.compatible = "renesas,rcar-gen3-can",
>  		.data = (void *)RCAR_SUPPORTED_CLOCKS,
>  	},
> +	{
> +		.compatible = "renesas,rzg-gen2-can",
> +		.data = (void *)RZG2_SUPPORTED_CLOCKS,
> +	},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, rcar_can_of_table);
> -- 
> 2.7.4
> 

Re: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support

[Simon Horman]

On Thu, Aug 23, 2018 at 02:07:33PM +0100, Fabrizio Castro wrote:
> Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
> generic bindings.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> Reviewed-by: Biju Das <biju.das@bp.renesas.com>

Are we sure these clocks are for all RZ/G2 SoCs?
If so:

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

> ---
> 
> This patch applies on next-20180823
> 
>  Documentation/devicetree/bindings/net/can/rcar_can.txt | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> index 94a7f33..ae8fccc 100644
> --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> @@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
>  Required properties:
>  - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
>  	      "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
> +	      "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.
>  	      "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
>  	      "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
>  	      "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> @@ -17,6 +18,7 @@ Required properties:
>  	      "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
>  	      compatible device.
>  	      "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> +	      "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.
>  	      When compatible with the generic version, nodes must list the
>  	      SoC-specific version corresponding to the platform first
>  	      followed by the generic version.
> @@ -24,7 +26,9 @@ Required properties:
>  - reg: physical base address and size of the R-Car CAN register map.
>  - interrupts: interrupt specifier for the sole interrupt.
>  - clocks: phandles and clock specifiers for 3 CAN clock inputs.
> -- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> +- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
> +	       3 clock input name strings for every other SoC: "clkp1", "clkp2",
> +	       "can_clk".
>  - pinctrl-0: pin control group to be used for this controller.
>  - pinctrl-names: must be "default".
>  
> @@ -41,8 +45,9 @@ using the below properties:
>  Optional properties:
>  - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
>  			    <0x0> (default) : Peripheral clock (clkp1)
> -			    <0x1> : Peripheral clock (clkp2)
> -			    <0x3> : Externally input clock
> +			    <0x1> : Peripheral clock (clkp2) (not supported by
> +				    RZ/G2 devices)
> +			    <0x3> : External input clock
>  
>  Example
>  -------
> -- 
> 2.7.4
> 

RE: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support

[Fabrizio Castro]

Hello Simon,

Thank you for your feedback!

> Subject: Re: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support
>
> On Thu, Aug 23, 2018 at 02:07:32PM +0100, Fabrizio Castro wrote:
> > RZ/G2 devices don't have clkp2, therefore this commit adds a
> > generic compatible string for them to allow for proper checking
> > during probe.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
>
>
> Are we sure these clocks are for all RZ/G2 SoCs?

Section 52.1.1 of the HW manual and Figure 52.1 state this.

Thanks,
Fab

>
> If so
>
>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>
>
> > ---
> >
> > This patch applies on linux-can-next-for-4.19-20180727
> >
> >  drivers/net/can/rcar/rcar_can.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> > index fbd9284..397208e 100644
> > --- a/drivers/net/can/rcar/rcar_can.c
> > +++ b/drivers/net/can/rcar/rcar_can.c
> > @@ -27,6 +27,7 @@
> >
> >  #define RCAR_SUPPORTED_CLOCKS(BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
> >   BIT(CLKR_CLKEXT))
> > +#define RZG2_SUPPORTED_CLOCKS(BIT(CLKR_CLKP1) | BIT(CLKR_CLKEXT))
> >
> >  /* Mailbox configuration:
> >   * mailbox 60 - 63 - Rx FIFO mailboxes
> > @@ -933,6 +934,10 @@ static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> >  .compatible = "renesas,rcar-gen3-can",
> >  .data = (void *)RCAR_SUPPORTED_CLOCKS,
> >  },
> > +{
> > +.compatible = "renesas,rzg-gen2-can",
> > +.data = (void *)RZG2_SUPPORTED_CLOCKS,
> > +},
> >  { }
> >  };
> >  MODULE_DEVICE_TABLE(of, rcar_can_of_table);
> > --
> > 2.7.4
> >



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support

[Fabrizio Castro]

Hello Simon,

Thank you for your feedback!

> Subject: Re: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support
>
> On Thu, Aug 23, 2018 at 02:07:33PM +0100, Fabrizio Castro wrote:
> > Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
> > generic bindings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
>
> Are we sure these clocks are for all RZ/G2 SoCs?

Yeah, that's the expectation.

Thanks,
Fab

> If so:
>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>
> > ---
> >
> > This patch applies on next-20180823
> >
> >  Documentation/devicetree/bindings/net/can/rcar_can.txt | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > index 94a7f33..ae8fccc 100644
> > --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > @@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
> >  Required properties:
> >  - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
> >        "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
> > +      "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.
> >        "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
> >        "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
> >        "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> > @@ -17,6 +18,7 @@ Required properties:
> >        "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
> >        compatible device.
> >        "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> > +      "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.
> >        When compatible with the generic version, nodes must list the
> >        SoC-specific version corresponding to the platform first
> >        followed by the generic version.
> > @@ -24,7 +26,9 @@ Required properties:
> >  - reg: physical base address and size of the R-Car CAN register map.
> >  - interrupts: interrupt specifier for the sole interrupt.
> >  - clocks: phandles and clock specifiers for 3 CAN clock inputs.
> > -- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> > +- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
> > +       3 clock input name strings for every other SoC: "clkp1", "clkp2",
> > +       "can_clk".
> >  - pinctrl-0: pin control group to be used for this controller.
> >  - pinctrl-names: must be "default".
> >
> > @@ -41,8 +45,9 @@ using the below properties:
> >  Optional properties:
> >  - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
> >      <0x0> (default) : Peripheral clock (clkp1)
> > -    <0x1> : Peripheral clock (clkp2)
> > -    <0x3> : Externally input clock
> > +    <0x1> : Peripheral clock (clkp2) (not supported by
> > +    RZ/G2 devices)
> > +    <0x3> : External input clock
> >
> >  Example
> >  -------
> > --
> > 2.7.4
> >



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH 1/3][can-next] can: rcar_can: Fix erroneous registration

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> Assigning 2 to "renesas,can-clock-select" tricks the driver into
> registering the CAN interface, even though we don't want that.
> This patch fixes this problem and also allows for architectures
> missing some of the clocks (e.g. RZ/G2) to behave as expected.

I think the fix for the second issue is not needed (see my reply to the other
patch).

> Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> ---
>
> This patch applies on linux-can-next-for-4.19-20180727
>
>  drivers/net/can/rcar/rcar_can.c | 43 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> index 11662f4..fbd9284 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -21,9 +21,13 @@
>  #include <linux/clk.h>
>  #include <linux/can/platform/rcar_can.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>
>  #define RCAR_CAN_DRV_NAME      "rcar_can"
>
> +#define RCAR_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
> +                                BIT(CLKR_CLKEXT))
> +
>  /* Mailbox configuration:
>   * mailbox 60 - 63 - Rx FIFO mailboxes
>   * mailbox 56 - 59 - Tx FIFO mailboxes
> @@ -745,10 +749,12 @@ static int rcar_can_probe(struct platform_device *pdev)
>         u32 clock_select = CLKR_CLKP1;
>         int err = -ENODEV;
>         int irq;
> +       uintptr_t allowed_clks = RCAR_SUPPORTED_CLOCKS;
>
>         if (pdev->dev.of_node) {
>                 of_property_read_u32(pdev->dev.of_node,
>                                      "renesas,can-clock-select", &clock_select);


> +               allowed_clks = (uintptr_t)of_device_get_match_data(&pdev->dev);
>         } else {
>                 pdata = dev_get_platdata(&pdev->dev);
>                 if (!pdata) {
> @@ -789,7 +795,7 @@ static int rcar_can_probe(struct platform_device *pdev)
>                 goto fail_clk;
>         }
>
> -       if (clock_select >= ARRAY_SIZE(clock_names)) {
> +       if (!(BIT(clock_select) & allowed_clks)) {

Hence you can just use RCAR_SUPPORTED_CLOCKS directly,
or better, just check clock_names[clock_select] != NULL, ...

>                 err = -EINVAL;
>                 dev_err(&pdev->dev, "invalid CAN clock selected\n");
>                 goto fail_clk;
> @@ -899,13 +905,34 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
>
>  static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> -       { .compatible = "renesas,can-r8a7778" },
> -       { .compatible = "renesas,can-r8a7779" },
> -       { .compatible = "renesas,can-r8a7790" },
> -       { .compatible = "renesas,can-r8a7791" },
> -       { .compatible = "renesas,rcar-gen1-can" },
> -       { .compatible = "renesas,rcar-gen2-can" },
> -       { .compatible = "renesas,rcar-gen3-can" },
> +       {
> +               .compatible = "renesas,can-r8a7778",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,can-r8a7779",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,can-r8a7790",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,can-r8a7791",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,rcar-gen1-can",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,rcar-gen2-can",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
> +       {
> +               .compatible = "renesas,rcar-gen3-can",
> +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> +       },
>         { }

... and all of the above can dropped.

>  };
>  MODULE_DEVICE_TABLE(of, rcar_can_of_table);

BTW, why does the custom "renesas,can-clock-select" exist?
If guess the standard "assigned-clock-parents" wasn't suitable because there's
no actual defined clock for which you can change the parent?

Why do you need manual selection? Can't the driver just pick the most suitable
available clock, like other drivers (e.g. sh-sci) do?

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support

[Geert Uytterhoeven]

Hi Fabrizio,

(Usually the DT patch goes before the driver patch)

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> RZ/G2 devices don't have clkp2, therefore this commit adds a
> generic compatible string for them to allow for proper checking
> during probe.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>

Thanks for your patch!

> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -27,6 +27,7 @@
>
>  #define RCAR_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
>                                  BIT(CLKR_CLKEXT))
> +#define RZG2_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKEXT))
>
>  /* Mailbox configuration:
>   * mailbox 60 - 63 - Rx FIFO mailboxes
> @@ -933,6 +934,10 @@ static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
>                 .compatible = "renesas,rcar-gen3-can",
>                 .data = (void *)RCAR_SUPPORTED_CLOCKS,
>         },
> +       {
> +               .compatible = "renesas,rzg-gen2-can",
> +               .data = (void *)RZG2_SUPPORTED_CLOCKS,
> +       },
>         { }

I think this patch is not needed, cfr. my reply to the
first^H^H^H^H^Hthird patch
in your series.

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 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:>
> Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
> generic bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> Reviewed-by: Biju Das <biju.das@bp.renesas.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> @@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
>  Required properties:
>  - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
>               "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
> +             "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.

Looks good to me.

>               "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
>               "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
>               "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> @@ -17,6 +18,7 @@ Required properties:
>               "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
>               compatible device.
>               "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> +             "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.

AFAIK, the actual CAN module in RZ/G2M is fully compatible with the CAN
module in R-Car Gen3 SoCs. The lack of clkp2 is merely an integration
difference: as RZ/G2 SoCs do not have the CANFD module, and their CPG block
doesn't provide the CANFD clock (so the CAN device node in DT cannot refer
to that clock anyway).

Hence I don't think there's a need to introduce a "renesas,rzg-gen2-can"
compatible value.

>               When compatible with the generic version, nodes must list the
>               SoC-specific version corresponding to the platform first
>               followed by the generic version.
> @@ -24,7 +26,9 @@ Required properties:
>  - reg: physical base address and size of the R-Car CAN register map.
>  - interrupts: interrupt specifier for the sole interrupt.
>  - clocks: phandles and clock specifiers for 3 CAN clock inputs.

You still have "3" here. Perhaps
"Must contain a phandle and clock-specifier pair for each entry in
clock-names."?

> -- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> +- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
> +              3 clock input name strings for every other SoC: "clkp1", "clkp2",
> +              "can_clk".

OK.

> @@ -41,8 +45,9 @@ using the below properties:
>  Optional properties:
>  - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
>                             <0x0> (default) : Peripheral clock (clkp1)
> -                           <0x1> : Peripheral clock (clkp2)
> -                           <0x3> : Externally input clock
> +                           <0x1> : Peripheral clock (clkp2) (not supported by
> +                                   RZ/G2 devices)
> +                           <0x3> : External input clock

I already expressed my feelings about this property in my reply to the first
patch ;-)

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 1/3][can-next] can: rcar_can: Fix erroneous registration

[Fabrizio Castro]

Hello Geert,

I am sorry for the late reply.
Thank you for your feedback.

> Subject: Re: [PATCH 1/3][can-next] can: rcar_can: Fix erroneous registration
>
> Hi Fabrizio,
>
> On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > Assigning 2 to "renesas,can-clock-select" tricks the driver into
> > registering the CAN interface, even though we don't want that.
> > This patch fixes this problem and also allows for architectures
> > missing some of the clocks (e.g. RZ/G2) to behave as expected.
>
> I think the fix for the second issue is not needed (see my reply to the other
> patch).
>
> > Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > ---
> >
> > This patch applies on linux-can-next-for-4.19-20180727
> >
> >  drivers/net/can/rcar/rcar_can.c | 43 +++++++++++++++++++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> > index 11662f4..fbd9284 100644
> > --- a/drivers/net/can/rcar/rcar_can.c
> > +++ b/drivers/net/can/rcar/rcar_can.c
> > @@ -21,9 +21,13 @@
> >  #include <linux/clk.h>
> >  #include <linux/can/platform/rcar_can.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> >  #define RCAR_CAN_DRV_NAME      "rcar_can"
> >
> > +#define RCAR_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
> > +                                BIT(CLKR_CLKEXT))
> > +
> >  /* Mailbox configuration:
> >   * mailbox 60 - 63 - Rx FIFO mailboxes
> >   * mailbox 56 - 59 - Tx FIFO mailboxes
> > @@ -745,10 +749,12 @@ static int rcar_can_probe(struct platform_device *pdev)
> >         u32 clock_select = CLKR_CLKP1;
> >         int err = -ENODEV;
> >         int irq;
> > +       uintptr_t allowed_clks = RCAR_SUPPORTED_CLOCKS;
> >
> >         if (pdev->dev.of_node) {
> >                 of_property_read_u32(pdev->dev.of_node,
> >                                      "renesas,can-clock-select", &clock_select);
>
>
> > +               allowed_clks = (uintptr_t)of_device_get_match_data(&pdev->dev);
> >         } else {
> >                 pdata = dev_get_platdata(&pdev->dev);
> >                 if (!pdata) {
> > @@ -789,7 +795,7 @@ static int rcar_can_probe(struct platform_device *pdev)
> >                 goto fail_clk;
> >         }
> >
> > -       if (clock_select >= ARRAY_SIZE(clock_names)) {
> > +       if (!(BIT(clock_select) & allowed_clks)) {
>
> Hence you can just use RCAR_SUPPORTED_CLOCKS directly,
> or better, just check clock_names[clock_select] != NULL, ...

I rather use RCAR_SUPPORTED_CLOCKS then, it's safer. I'll send a v2 for you to look at.

>
> >                 err = -EINVAL;
> >                 dev_err(&pdev->dev, "invalid CAN clock selected\n");
> >                 goto fail_clk;
> > @@ -899,13 +905,34 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
> >  static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
> >
> >  static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> > -       { .compatible = "renesas,can-r8a7778" },
> > -       { .compatible = "renesas,can-r8a7779" },
> > -       { .compatible = "renesas,can-r8a7790" },
> > -       { .compatible = "renesas,can-r8a7791" },
> > -       { .compatible = "renesas,rcar-gen1-can" },
> > -       { .compatible = "renesas,rcar-gen2-can" },
> > -       { .compatible = "renesas,rcar-gen3-can" },
> > +       {
> > +               .compatible = "renesas,can-r8a7778",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,can-r8a7779",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,can-r8a7790",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,can-r8a7791",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,rcar-gen1-can",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,rcar-gen2-can",
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> > +       {
> > +               .compatible = "renesas,rcar-gen3-can",z
> > +               .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > +       },
> >         { }
>
> ... and all of the above can dropped.
>
> >  };
> >  MODULE_DEVICE_TABLE(of, rcar_can_of_table);
>
> BTW, why does the custom "renesas,can-clock-select" exist?
> If guess the standard "assigned-clock-parents" wasn't suitable because there's
> no actual defined clock for which you can change the parent?
>
> Why do you need manual selection? Can't the driver just pick the most suitable
> available clock, like other drivers (e.g. sh-sci) do?

Please have a look at 862e2b6af941 ("can: rcar_can: support all input clocks").
Maybe this could be improved in the future?

Thanks,
Fab

>
> 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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support

[Fabrizio Castro]

Hello Geert,

Thank you for your feedback.

> Subject: Re: [PATCH 2/3][can-next] can: rcar_can: Add RZ/G2 support
>
> Hi Fabrizio,
>
> (Usually the DT patch goes before the driver patch)
>
> On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > RZ/G2 devices don't have clkp2, therefore this commit adds a
> > generic compatible string for them to allow for proper checking
> > during probe.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/net/can/rcar/rcar_can.c
> > +++ b/drivers/net/can/rcar/rcar_can.c
> > @@ -27,6 +27,7 @@
> >
> >  #define RCAR_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
> >                                  BIT(CLKR_CLKEXT))
> > +#define RZG2_SUPPORTED_CLOCKS  (BIT(CLKR_CLKP1) | BIT(CLKR_CLKEXT))
> >
> >  /* Mailbox configuration:
> >   * mailbox 60 - 63 - Rx FIFO mailboxes
> > @@ -933,6 +934,10 @@ static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> >                 .compatible = "renesas,rcar-gen3-can",
> >                 .data = (void *)RCAR_SUPPORTED_CLOCKS,
> >         },
> > +       {
> > +               .compatible = "renesas,rzg-gen2-can",
> > +               .data = (void *)RZG2_SUPPORTED_CLOCKS,
> > +       },
> >         { }
>
> I think this patch is not needed, cfr. my reply to the
> first^H^H^H^H^Hthird patch
> in your series.

I am dropping this patch.

Thanks,
Fab

>
> 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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support

[Fabrizio Castro]

Hello Geert,

Thank you for your feedback.

> Subject: Re: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support
>
> Hi Fabrizio,
>
> On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:>
> > Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
> > generic bindings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > @@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
> >  Required properties:
> >  - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
> >               "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
> > +             "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.
>
> Looks good to me.
>
> >               "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
> >               "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
> >               "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> > @@ -17,6 +18,7 @@ Required properties:
> >               "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
> >               compatible device.
> >               "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> > +             "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.
>
> AFAIK, the actual CAN module in RZ/G2M is fully compatible with the CAN
> module in R-Car Gen3 SoCs. The lack of clkp2 is merely an integration
> difference: as RZ/G2 SoCs do not have the CANFD module, and their CPG block
> doesn't provide the CANFD clock (so the CAN device node in DT cannot refer
> to that clock anyway).
>
> Hence I don't think there's a need to introduce a "renesas,rzg-gen2-can"
> compatible value.

Agreed, will drop RZ/G2 specific compatible string.

>
> >               When compatible with the generic version, nodes must list the
> >               SoC-specific version corresponding to the platform first
> >               followed by the generic version.
> > @@ -24,7 +26,9 @@ Required properties:
> >  - reg: physical base address and size of the R-Car CAN register map.
> >  - interrupts: interrupt specifier for the sole interrupt.
> >  - clocks: phandles and clock specifiers for 3 CAN clock inputs.
>
> You still have "3" here. Perhaps
> "Must contain a phandle and clock-specifier pair for each entry in
> clock-names."?

Good spot, we overlooked it.

>
> > -- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> > +- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
> > +              3 clock input name strings for every other SoC: "clkp1", "clkp2",
> > +              "can_clk".
>
> OK.
>
> > @@ -41,8 +45,9 @@ using the below properties:
> >  Optional properties:
> >  - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
> >                             <0x0> (default) : Peripheral clock (clkp1)
> > -                           <0x1> : Peripheral clock (clkp2)
> > -                           <0x3> : Externally input clock
> > +                           <0x1> : Peripheral clock (clkp2) (not supported by
> > +                                   RZ/G2 devices)
> > +                           <0x3> : External input clock
>
> I already expressed my feelings about this property in my reply to the first
> patch ;-)

I know, I am not super happy about it either, maybe we will get a proper solution for this at some point in the future.

Thanks,
Fab

>
> 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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.