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

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

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

Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Chris Paterson <redacted>
Reviewed-by: Biju Das <redacted>
---

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

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 <redacted>
Signed-off-by: Chris Paterson <redacted>
---

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

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 <redacted>
Signed-off-by: Chris Paterson <redacted>
---

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

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 <redacted>
Signed-off-by: Chris Paterson <redacted>
Reviewed-by: Biju Das <redacted>

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

Reviewed-by: Simon Horman <redacted>

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:

quoted

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

Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Chris Paterson <redacted>
Reviewed-by: Biju Das <redacted>

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

Yeah, that's the expectation.

Thanks,
Fab

If so:

Reviewed-by: Simon Horman <redacted>

quoted

---

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.

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 <redacted>
Signed-off-by: Chris Paterson <redacted>

Reviewed-by: Simon Horman <redacted>

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 <redacted>
Signed-off-by: Chris Paterson <redacted>

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

If so


Reviewed-by: Simon Horman <redacted>

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:

quoted

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 <redacted>
Signed-off-by: Chris Paterson <redacted>

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

quoted

---

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.

Hi Fabrizio,

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
[off-list ref] wrote:>

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

Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Chris Paterson <redacted>
Reviewed-by: Biju Das <redacted>

Thanks for your patch!

Looks good to me.

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.

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.

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

Hi Fabrizio,

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
[off-list ref] 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).

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

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

Hi Fabrizio,

(Usually the DT patch goes before the driver patch)

On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
[off-list ref] 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 <redacted>
Signed-off-by: Chris Paterson <redacted>

Thanks for your patch!

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

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
[off-list ref] wrote:>

quoted

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

Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Chris Paterson <redacted>
Reviewed-by: Biju Das <redacted>

Thanks for your patch!

quoted

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

quoted

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

quoted

              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.

quoted

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

quoted

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

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
[off-list ref] wrote:

quoted

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

quoted

Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Chris Paterson <redacted>
---

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

quoted

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

quoted

                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.

quoted

 };
 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.

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
[off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Chris Paterson <redacted>

Thanks for your patch!

quoted

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