[PATCH v2 1/7] dt-bindings: arm: Document Renesas R-Car M3-N-based ULCB board

[PATCH v2 1/7] dt-bindings: arm: Document Renesas R-Car M3-N-based ULCB board

[Eugeniu Rosca]

In harmony with ATF and U-Boot outputs [1] and [2], the new board is
based on M3-N revision ES1.1 and the amount of memory present on SiP
is 2GiB, contiguously addressed.

The amount of RAM is mentioned based on the assumption that it is
encoded in the board id/string. There is some evidence supporting this
in form of last-digit-mismatch between two R-Car H3 ES2.0 ULCB board
ids, one with 4GiB and one with 8GiB of RAM (see [3]).

[1] BL2: R-Car Gen3 Initial Program Loader(CA57) Rev.1.0.21
    BL2: PRR is R-Car M3N Ver.1.1

[2] U-Boot 2015.04-00295-*
    CPU: Renesas Electronics R8A77965 rev 1.1
    ---8<----
    DRAM:  1.9 GiB
    Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB
    ---8<----

[3] https://patchwork.kernel.org/patch/10555957/#22169325

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - [Jacopo Mondi] Emphasized the fact the amount of RAM is encoded in
   the board id, so documenting it *is* relevant for this commit.
 - [Simon Horman]
   - Renamed board name 's/M3-N ULCB/M3NULCB/'
   - Renamed compatible string 's/renesas,ulcb/renesas,m3nulcb/'
 - Pushed all dt-bindings patches to the beginning of the series.
---
 Documentation/devicetree/bindings/arm/shmobile.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/shmobile.txt b/Documentation/devicetree/bindings/arm/shmobile.txt
index d8cf740132c6..03a314c7989d 100644
--- a/Documentation/devicetree/bindings/arm/shmobile.txt
+++ b/Documentation/devicetree/bindings/arm/shmobile.txt
@@ -106,6 +106,8 @@ Boards:
     compatible = "renesas,lager", "renesas,r8a7790"
   - M3ULCB (R-Car Starter Kit Pro, RTP0RC7796SKBX0010SA09 (M3 ES1.0))
     compatible = "renesas,m3ulcb", "renesas,r8a7796"
+  - M3NULCB (R-Car Starter Kit Pro, RTP0RC77965SKBX010SA00 (M3-N ES1.1))
+    compatible = "renesas,m3nulcb", "renesas,r8a77965"
   - Marzen (R0P7779A00010S)
     compatible = "renesas,marzen", "renesas,r8a7779"
   - Porter (M2-LCDP)
-- 
2.18.0

[PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Eugeniu Rosca]

Document the support for rcar_can on R8A77965 SoC devices.
Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
"assigned-clock-rates" properties (thanks, Sergei). Rewrap text.

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - [Kieran Bingham] Simplified commit description. Rewrapped text.
 - [Sergei Shtylyov] Replaced footnotes with inline text.
 - Pushed all dt-bindings patches to the beginning of the series.
---
 Documentation/devicetree/bindings/net/can/rcar_can.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
index 94a7f33ac5e9..60daa878c9a2 100644
--- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
+++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
@@ -13,6 +13,7 @@ Required properties:
 	      "renesas,can-r8a7794" if CAN controller is a part of R8A7794 SoC.
 	      "renesas,can-r8a7795" if CAN controller is a part of R8A7795 SoC.
 	      "renesas,can-r8a7796" if CAN controller is a part of R8A7796 SoC.
+	      "renesas,can-r8a77965" if CAN controller is a part of R8A77965 SoC.
 	      "renesas,rcar-gen1-can" for a generic R-Car Gen1 compatible device.
 	      "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
 	      compatible device.
@@ -28,11 +29,10 @@ Required properties:
 - pinctrl-0: pin control group to be used for this controller.
 - pinctrl-names: must be "default".
 
-Required properties for "renesas,can-r8a7795" and "renesas,can-r8a7796"
-compatible:
-In R8A7795 and R8A7796 SoCs, "clkp2" can be CANFD clock. This is a div6 clock
-and can be used by both CAN and CAN FD controller at the same time. It needs to
-be scaled to maximum frequency if any of these controllers use it. This is done
+Required properties for R8A7795, R8A7796 and R8A77965:
+For the denoted SoCs, "clkp2" can be CANFD clock. This is a div6 clock and can
+be used by both CAN and CAN FD controller at the same time. It needs to be
+scaled to maximum frequency if any of these controllers use it. This is done
 using the below properties:
 
 - assigned-clocks: phandle of clkp2(CANFD) clock.
-- 
2.18.0

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:44PM +0200, Eugeniu Rosca wrote:
> Document the support for rcar_can on R8A77965 SoC devices.
> Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
> "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> Changes in v2:
>  - [Kieran Bingham] Simplified commit description. Rewrapped text.
>  - [Sergei Shtylyov] Replaced footnotes with inline text.
>  - Pushed all dt-bindings patches to the beginning of the series.
> ---
>  Documentation/devicetree/bindings/net/can/rcar_can.txt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

As this is a CAN patch I believe it should be:

  * Targeted at the net-next tree.

  * Have net-next in the subject prefix

    e.g.:

    "[PATCH v3 net-next] dt-bindings: can: rcar_can: document r8a77965 support"

    * Sent to:
      Wolfgang Grandegger <wg@grandegger.com>
      Marc Kleine-Budde <mkl@pengutronix.de>
      "David S. Miller" <davem@davemloft.net>
      linux-can@vger.kernel.org
      netdev@vger.kernel.org

As it is a DT binding patch I believe it should also be sent to:
  Rob Herring <robh+dt@kernel.org>
  Mark Rutland <mark.rutland@arm.com>
  devicetree@vger.kernel.org

And as it is related to Renesas SoCs it should also be sent to:
  Simon Horman <horms@verge.net.au>
  Magnus Damm <damm+renesas@opensource.se>
  linux-renesas-soc@vger.kernel.org

Sending it to more people is ok too :)

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Eugeniu Rosca]

Hi Simon,

On Fri, Aug 17, 2018 at 11:04:03AM +0200, Simon Horman wrote:
> On Sun, Aug 12, 2018 at 03:31:44PM +0200, Eugeniu Rosca wrote:
> > Document the support for rcar_can on R8A77965 SoC devices.
> > Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
> > "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> > Changes in v2:
> >  - [Kieran Bingham] Simplified commit description. Rewrapped text.
> >  - [Sergei Shtylyov] Replaced footnotes with inline text.
> >  - Pushed all dt-bindings patches to the beginning of the series.
> > ---
> >  Documentation/devicetree/bindings/net/can/rcar_can.txt | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> 
> As this is a CAN patch I believe it should be:
> 
>   * Targeted at the net-next tree.
> 
>   * Have net-next in the subject prefix
> 
>     e.g.:
> 
>     "[PATCH v3 net-next] dt-bindings: can: rcar_can: document r8a77965 support"

I will re-spin this particular patch as v3 with your Reviewed-by and all
the suggested names included in the "Sent to" field.

> 
>     * Sent to:
>       Wolfgang Grandegger <wg@grandegger.com>
>       Marc Kleine-Budde <mkl@pengutronix.de>
>       "David S. Miller" <davem@davemloft.net>
>       linux-can@vger.kernel.org
>       netdev@vger.kernel.org
> 
> As it is a DT binding patch I believe it should also be sent to:
>   Rob Herring <robh+dt@kernel.org>
>   Mark Rutland <mark.rutland@arm.com>
>   devicetree@vger.kernel.org
> 
> And as it is related to Renesas SoCs it should also be sent to:
>   Simon Horman <horms@verge.net.au>
>   Magnus Damm <damm+renesas@opensource.se>
>   linux-renesas-soc@vger.kernel.org
> 
> Sending it to more people is ok too :)

Best regards,
Eugeniu.

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Kieran Bingham]

Hi Eugeniu

Thank you for the patch.

On 12/08/18 14:31, Eugeniu Rosca wrote:
> Document the support for rcar_can on R8A77965 SoC devices.
> Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
> "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.

I don't think you needed to say you rewrapped the text in the commit log
- but it's fine :)


> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> Changes in v2:
>  - [Kieran Bingham] Simplified commit description. Rewrapped text.
>  - [Sergei Shtylyov] Replaced footnotes with inline text.
>  - Pushed all dt-bindings patches to the beginning of the series.
> ---
>  Documentation/devicetree/bindings/net/can/rcar_can.txt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> index 94a7f33ac5e9..60daa878c9a2 100644
> --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> @@ -13,6 +13,7 @@ Required properties:
>  	      "renesas,can-r8a7794" if CAN controller is a part of R8A7794 SoC.
>  	      "renesas,can-r8a7795" if CAN controller is a part of R8A7795 SoC.
>  	      "renesas,can-r8a7796" if CAN controller is a part of R8A7796 SoC.
> +	      "renesas,can-r8a77965" if CAN controller is a part of R8A77965 SoC.
>  	      "renesas,rcar-gen1-can" for a generic R-Car Gen1 compatible device.
>  	      "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
>  	      compatible device.
> @@ -28,11 +29,10 @@ Required properties:
>  - pinctrl-0: pin control group to be used for this controller.
>  - pinctrl-names: must be "default".
>  
> -Required properties for "renesas,can-r8a7795" and "renesas,can-r8a7796"
> -compatible:
> -In R8A7795 and R8A7796 SoCs, "clkp2" can be CANFD clock. This is a div6 clock
> -and can be used by both CAN and CAN FD controller at the same time. It needs to
> -be scaled to maximum frequency if any of these controllers use it. This is done
> +Required properties for R8A7795, R8A7796 and R8A77965:
> +For the denoted SoCs, "clkp2" can be CANFD clock. This is a div6 clock and can
> +be used by both CAN and CAN FD controller at the same time. It needs to be
> +scaled to maximum frequency if any of these controllers use it. This is done
>  using the below properties:
>  
>  - assigned-clocks: phandle of clkp2(CANFD) clock.
> 

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Eugeniu Rosca]

Hello Kieran,

On Fri, Aug 17, 2018 at 02:44:25PM +0100, Kieran Bingham wrote:
> Hi Eugeniu
> 
> Thank you for the patch.
> 
> On 12/08/18 14:31, Eugeniu Rosca wrote:
> > Document the support for rcar_can on R8A77965 SoC devices.
> > Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
> > "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.
> 
> I don't think you needed to say you rewrapped the text in the commit log
> - but it's fine :)

IMHO "Rewrap text" is pretty much from the same category as "no
functional change was intended". As a reviewer, I would take these
details in the commit description any day (and sometimes I would NAK a
patch which lacks these details), since they precisely express the goals
set by the author and make reviewer's life easier.

But, of course, preferences vary and therefore I won't elaborate on that
too much.

> 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 

Best regards,
Eugeniu.

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Kieran Bingham]

Hi Eugeniu,

On 17/08/18 16:56, Eugeniu Rosca wrote:
> Hello Kieran,
> 
> On Fri, Aug 17, 2018 at 02:44:25PM +0100, Kieran Bingham wrote:
>> Hi Eugeniu
>>
>> Thank you for the patch.
>>
>> On 12/08/18 14:31, Eugeniu Rosca wrote:
>>> Document the support for rcar_can on R8A77965 SoC devices.
>>> Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
>>> "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.
>>
>> I don't think you needed to say you rewrapped the text in the commit log
>> - but it's fine :)
> 
> IMHO "Rewrap text" is pretty much from the same category as "no
> functional change was intended".

Indeed, but in this instance - there was a functional change. You
modified the paragraph.  In fact, mentioning that you have rewrapped the
text, thus implying that you have made no functional change might cause
a reviewer not to look deeper at the actual differences?


> As a reviewer, I would take these
> details in the commit description any day (and sometimes I would NAK a
> patch which lacks these details), since they precisely express the goals
> set by the author and make reviewer's life easier.
> 
> But, of course, preferences vary and therefore I won't elaborate on that
> too much.

If this was a separate hunk, which you had re-wrapped without making a
change to - I would absolutely agree with you here. The 'rewrapping'
should be mentioned in the commit message, but this in relation to a
paragraph which you had modified.

IMO - if you modify a paragraph of text, rewrapping to make sure it fits
the constraints is part of that modification ... but ... yes we are
debating minor details and preferences here ;) - I have no objection to
you mentioning it.

Regards

Kieran

> 
>>
>>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
> 
> Best regards,
> Eugeniu.
> 

Re: [PATCH v2 2/7] dt-bindings: can: rcar_can: document r8a77965 can support

[Eugeniu Rosca]

Hi Kieran,

On Fri, Aug 17, 2018 at 05:10:06PM +0100, Kieran Bingham wrote:
> Hi Eugeniu,
> 
> On 17/08/18 16:56, Eugeniu Rosca wrote:
> > Hello Kieran,
> > 
> > On Fri, Aug 17, 2018 at 02:44:25PM +0100, Kieran Bingham wrote:
> >> Hi Eugeniu
> >>
> >> Thank you for the patch.
> >>
> >> On 12/08/18 14:31, Eugeniu Rosca wrote:
> >>> Document the support for rcar_can on R8A77965 SoC devices.
> >>> Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
> >>> "assigned-clock-rates" properties (thanks, Sergei). Rewrap text.
> >>
> >> I don't think you needed to say you rewrapped the text in the commit log
> >> - but it's fine :)
> > 
> > IMHO "Rewrap text" is pretty much from the same category as "no
> > functional change was intended".
> 
> Indeed, but in this instance - there was a functional change. You
> modified the paragraph.  In fact, mentioning that you have rewrapped the
> text, thus implying that you have made no functional change might cause
> a reviewer not to look deeper at the actual differences?
> 
> 
> > As a reviewer, I would take these
> > details in the commit description any day (and sometimes I would NAK a
> > patch which lacks these details), since they precisely express the goals
> > set by the author and make reviewer's life easier.
> > 
> > But, of course, preferences vary and therefore I won't elaborate on that
> > too much.
> 
> If this was a separate hunk, which you had re-wrapped without making a
> change to - I would absolutely agree with you here. The 'rewrapping'
> should be mentioned in the commit message, but this in relation to a
> paragraph which you had modified.
> 
> IMO - if you modify a paragraph of text, rewrapping to make sure it fits
> the constraints is part of that modification ... but ... yes we are
> debating minor details and preferences here ;) - I have no objection to
> you mentioning it.

Ok, I get your point. I have to tune my auto-pilot to avoid writing down
obvious things in the commit descriptions.

> 
> Regards
> 
> Kieran

Thanks,
Eugeniu.

[PATCH v2 3/7] pinctrl: sh-pfc: r8a77965: Add HSCIF0 pins, groups, and functions

[Eugeniu Rosca]

According to R-Car Gen3 HW manual Rev.1.00 Apr 2018, M3-N SoC implements
five (0..4) HSCIF channels, similar to H3, M3-W and E3.

The story behind this patch is tackling below dmesg warnings, which pop
up when booting M3NULCB Kingfisher board:

$ dmesg | grep sh-pfc
sh-pfc e6060000.pin-controller: r8a77965_pfc support registered
sh-pfc e6060000.pin-controller: function 'hscif0' not supported
sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
sh-pfc e6060000.pin-controller: function 'hscif0' not supported
sh-pfc e6060000.pin-controller: invalid function hscif0 in map table

To fix them, extract the HSCIF0 part from below v4.15-rc1 commits:
 - commit 7a362e3488cb ("pinctrl: sh-pfc: r8a7795: Add HSCIF pins, groups, and functions")
 - commit 0e4e4999aac1 ("pinctrl: sh-pfc: r8a7796: Add HSCIF pins, groups, and functions")

Note that `checkpatch --strict` throws several "CHECK: Please use a
blank line after function/struct/union/enum declarations", which are
ignored for the sake of staying in sync with the aforementioned commits.

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - Newly added.
 - IMHO mirroring the mentioned H3 and M3-W sh-pfc commits *entirely* is
   a better option to avoid work fragmentation, but I leave this
   decision to the maintainer.
---
 drivers/pinctrl/sh-pfc/pfc-r8a77965.c | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77965.c b/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
index d2bbee656381..fe16d194f69b 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
@@ -1758,6 +1758,28 @@ static const unsigned int du_disp_mux[] = {
 	DU_DISP_MARK,
 };
 
+/* - HSCIF0 ----------------------------------------------------------------- */
+static const unsigned int hscif0_data_pins[] = {
+	/* RX, TX */
+	RCAR_GP_PIN(5, 13), RCAR_GP_PIN(5, 14),
+};
+static const unsigned int hscif0_data_mux[] = {
+	HRX0_MARK, HTX0_MARK,
+};
+static const unsigned int hscif0_clk_pins[] = {
+	/* SCK */
+	RCAR_GP_PIN(5, 12),
+};
+static const unsigned int hscif0_clk_mux[] = {
+	HSCK0_MARK,
+};
+static const unsigned int hscif0_ctrl_pins[] = {
+	/* RTS, CTS */
+	RCAR_GP_PIN(5, 16), RCAR_GP_PIN(5, 15),
+};
+static const unsigned int hscif0_ctrl_mux[] = {
+	HRTS0_N_MARK, HCTS0_N_MARK,
+};
 /* - I2C -------------------------------------------------------------------- */
 static const unsigned int i2c1_a_pins[] = {
 	/* SDA, SCL */
@@ -3169,6 +3191,9 @@ static const struct sh_pfc_pin_group pinmux_groups[] = {
 	SH_PFC_PIN_GROUP(du_oddf),
 	SH_PFC_PIN_GROUP(du_cde),
 	SH_PFC_PIN_GROUP(du_disp),
+	SH_PFC_PIN_GROUP(hscif0_data),
+	SH_PFC_PIN_GROUP(hscif0_clk),
+	SH_PFC_PIN_GROUP(hscif0_ctrl),
 	SH_PFC_PIN_GROUP(i2c1_a),
 	SH_PFC_PIN_GROUP(i2c1_b),
 	SH_PFC_PIN_GROUP(i2c2_a),
@@ -3379,6 +3404,12 @@ static const char * const du_groups[] = {
 	"du_disp",
 };
 
+static const char * const hscif0_groups[] = {
+	"hscif0_data",
+	"hscif0_clk",
+	"hscif0_ctrl",
+};
+
 static const char * const i2c1_groups[] = {
 	"i2c1_a",
 	"i2c1_b",
@@ -3651,6 +3682,7 @@ static const char * const usb30_groups[] = {
 static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(avb),
 	SH_PFC_FUNCTION(du),
+	SH_PFC_FUNCTION(hscif0),
 	SH_PFC_FUNCTION(i2c1),
 	SH_PFC_FUNCTION(i2c2),
 	SH_PFC_FUNCTION(i2c6),
-- 
2.18.0

Re: [PATCH v2 3/7] pinctrl: sh-pfc: r8a77965: Add HSCIF0 pins, groups, and functions

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:45PM +0200, Eugeniu Rosca wrote:
> According to R-Car Gen3 HW manual Rev.1.00 Apr 2018, M3-N SoC implements
> five (0..4) HSCIF channels, similar to H3, M3-W and E3.
> 
> The story behind this patch is tackling below dmesg warnings, which pop
> up when booting M3NULCB Kingfisher board:
> 
> $ dmesg | grep sh-pfc
> sh-pfc e6060000.pin-controller: r8a77965_pfc support registered
> sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
> sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
> 
> To fix them, extract the HSCIF0 part from below v4.15-rc1 commits:
>  - commit 7a362e3488cb ("pinctrl: sh-pfc: r8a7795: Add HSCIF pins, groups, and functions")
>  - commit 0e4e4999aac1 ("pinctrl: sh-pfc: r8a7796: Add HSCIF pins, groups, and functions")
> 
> Note that `checkpatch --strict` throws several "CHECK: Please use a
> blank line after function/struct/union/enum declarations", which are
> ignored for the sake of staying in sync with the aforementioned commits.
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

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

Re: [PATCH v2 3/7] pinctrl: sh-pfc: r8a77965: Add HSCIF0 pins, groups, and functions

[Geert Uytterhoeven]

Hi Eugeniu,

On Sun, Aug 12, 2018 at 3:33 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> According to R-Car Gen3 HW manual Rev.1.00 Apr 2018, M3-N SoC implements
> five (0..4) HSCIF channels, similar to H3, M3-W and E3.
>
> The story behind this patch is tackling below dmesg warnings, which pop
> up when booting M3NULCB Kingfisher board:
>
> $ dmesg | grep sh-pfc
> sh-pfc e6060000.pin-controller: r8a77965_pfc support registered
> sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
> sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
>
> To fix them, extract the HSCIF0 part from below v4.15-rc1 commits:
>  - commit 7a362e3488cb ("pinctrl: sh-pfc: r8a7795: Add HSCIF pins, groups, and functions")
>  - commit 0e4e4999aac1 ("pinctrl: sh-pfc: r8a7796: Add HSCIF pins, groups, and functions")
>
> Note that `checkpatch --strict` throws several "CHECK: Please use a
> blank line after function/struct/union/enum declarations", which are
> ignored for the sake of staying in sync with the aforementioned commits.
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks for your patch, but please see commit 7628fa811b8af571 ("pinctrl:
sh-pfc: r8a77965: Add HSCIF pins, groups, and functions") in v4.19-rc1.

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 v2 3/7] pinctrl: sh-pfc: r8a77965: Add HSCIF0 pins, groups, and functions

[Eugeniu Rosca]

Hi Geert,

On Mon, Aug 27, 2018 at 05:07:39PM +0200, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> On Sun, Aug 12, 2018 at 3:33 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > According to R-Car Gen3 HW manual Rev.1.00 Apr 2018, M3-N SoC implements
> > five (0..4) HSCIF channels, similar to H3, M3-W and E3.
> >
> > The story behind this patch is tackling below dmesg warnings, which pop
> > up when booting M3NULCB Kingfisher board:
> >
> > $ dmesg | grep sh-pfc
> > sh-pfc e6060000.pin-controller: r8a77965_pfc support registered
> > sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> > sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
> > sh-pfc e6060000.pin-controller: function 'hscif0' not supported
> > sh-pfc e6060000.pin-controller: invalid function hscif0 in map table
> >
> > To fix them, extract the HSCIF0 part from below v4.15-rc1 commits:
> >  - commit 7a362e3488cb ("pinctrl: sh-pfc: r8a7795: Add HSCIF pins, groups, and functions")
> >  - commit 0e4e4999aac1 ("pinctrl: sh-pfc: r8a7796: Add HSCIF pins, groups, and functions")
> >
> > Note that `checkpatch --strict` throws several "CHECK: Please use a
> > blank line after function/struct/union/enum declarations", which are
> > ignored for the sake of staying in sync with the aforementioned commits.
> >
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Thanks for your patch, but please see commit 7628fa811b8af571 ("pinctrl:
> sh-pfc: r8a77965: Add HSCIF pins, groups, and functions") in v4.19-rc1.

Thanks for pointing out. Please, feel free to drop the patch. Its
purpose was to build a self-contained (no new compiler/dmesg
warnings/errors) patch series assuming v4.18.

> 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

Best regards,
Eugeniu.

[PATCH v2 4/7] arm64: dts: renesas: r8a77965: m3nulcb: Initial device tree

[Eugeniu Rosca]

Allow the bare M3-N-based ULCB board to boot.

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes in v2:
 - [Jacopo Mondi] Removed redundant license text. Added Reviewed-by.
 - [Simon Horman]
   - Renamed DTS 's/r8a77965-ulcb.dts/r8a77965-m3nulcb.dts/'
   - Renamed board name 's/M3-N ULCB/M3NULCB/'
   - Renamed compatible string 's/renesas,ulcb/renesas,m3nulcb/'
   - Adapted Makefile and commit summary line 's/ulcb/m3nulcb/'
---
 arch/arm64/boot/dts/renesas/Makefile          |  1 +
 .../boot/dts/renesas/r8a77965-m3nulcb.dts     | 33 +++++++++++++++++++
 2 files changed, 34 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/r8a77965-m3nulcb.dts

diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 9e2394bc3c62..eb158d1f90e9 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -8,6 +8,7 @@ dtb-$(CONFIG_ARCH_R8A7796) += r8a7796-salvator-x.dtb r8a7796-m3ulcb.dtb
 dtb-$(CONFIG_ARCH_R8A7796) += r8a7796-m3ulcb-kf.dtb
 dtb-$(CONFIG_ARCH_R8A7796) += r8a7796-salvator-xs.dtb
 dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-salvator-x.dtb r8a77965-salvator-xs.dtb
+dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-m3nulcb.dtb
 dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-eagle.dtb r8a77970-v3msk.dtb
 dtb-$(CONFIG_ARCH_R8A77980) += r8a77980-condor.dtb r8a77980-v3hsk.dtb
 dtb-$(CONFIG_ARCH_R8A77990) += r8a77990-ebisu.dtb
diff --git a/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb.dts b/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb.dts
new file mode 100644
index 000000000000..964078b6cc49
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb.dts
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the M3NULCB (R-Car Starter Kit Pro) board
+ *
+ * Copyright (C) 2018 Renesas Electronics Corp.
+ * Copyright (C) 2018 Cogent Embedded, Inc.
+ */
+
+/dts-v1/;
+#include "r8a77965.dtsi"
+#include "ulcb.dtsi"
+
+/ {
+	model = "Renesas M3NULCB board based on r8a77965";
+	compatible = "renesas,m3nulcb", "renesas,r8a77965";
+
+	memory@48000000 {
+		device_type = "memory";
+		/* first 128MB is reserved for secure area. */
+		reg = <0x0 0x48000000 0x0 0x78000000>;
+	};
+};
+
+&du {
+	clocks = <&cpg CPG_MOD 724>,
+		 <&cpg CPG_MOD 723>,
+		 <&cpg CPG_MOD 721>,
+		 <&versaclock5 1>,
+		 <&versaclock5 3>,
+		 <&versaclock5 2>;
+	clock-names = "du.0", "du.1", "du.3",
+		      "dclkin.0", "dclkin.1", "dclkin.3";
+};
-- 
2.18.0

Re: [PATCH v2 4/7] arm64: dts: renesas: r8a77965: m3nulcb: Initial device tree

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:46PM +0200, Eugeniu Rosca wrote:
> Allow the bare M3-N-based ULCB board to boot.
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks Eugeniu,

applied for v4.20.

[PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Eugeniu Rosca]

According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
interfaces, similar to H3, M3-W and other SoCs from the same family.

Add CAN placeholder nodes to avoid below DTC errors:
Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found

These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
Fix them beforehand.

CAN support is inspired from below commits:
 - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
 - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
 - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - [Kieran Bingham] Improved commit description:
   - Referenced the newer HW manual rev1.00 instead of rev0.55E.
   - Kept the "true story" behind the patch. Just made it more clear.
 - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
   (no CAN testing was done to validate the DTS configuration).
---
 arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
index 486aecacb22a..4da479d3c226 100644
--- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -656,6 +656,22 @@
 			status = "disabled";
 		};
 
+		can0: can@e6c30000 {
+			compatible = "renesas,can-r8a77965",
+				     "renesas,rcar-gen3-can";
+			reg = <0 0xe6c30000 0 0x1000>;
+			/* placeholder */
+			status = "disabled";
+		};
+
+		can1: can@e6c38000 {
+			compatible = "renesas,can-r8a77965",
+				     "renesas,rcar-gen3-can";
+			reg = <0 0xe6c38000 0 0x1000>;
+			/* placeholder */
+			status = "disabled";
+		};
+
 		pwm0: pwm@e6e30000 {
 			compatible = "renesas,pwm-r8a77965", "renesas,pwm-rcar";
 			reg = <0 0xe6e30000 0 8>;
-- 
2.18.0

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Kieran Bingham]

Hi Eugeniu,

Thank you for the patch,

On 12/08/18 14:31, Eugeniu Rosca wrote:
> According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> interfaces, similar to H3, M3-W and other SoCs from the same family.
> 
> Add CAN placeholder nodes to avoid below DTC errors:
> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> 
> These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> Fix them beforehand.
> 
> CAN support is inspired from below commits:
>  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
>  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
>  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
> Changes in v2:
>  - [Kieran Bingham] Improved commit description:
>    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
>    - Kept the "true story" behind the patch. Just made it more clear.
>  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
>    (no CAN testing was done to validate the DTS configuration).
> ---
>  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> index 486aecacb22a..4da479d3c226 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> @@ -656,6 +656,22 @@
>  			status = "disabled";
>  		};
>  
> +		can0: can@e6c30000 {
> +			compatible = "renesas,can-r8a77965",
> +				     "renesas,rcar-gen3-can";
> +			reg = <0 0xe6c30000 0 0x1000>;
> +			/* placeholder */
> +			status = "disabled";
> +		};

This is probably more detail than is needed for a placeholder, but it
looks correct so I think this is fine.

--
Kieran



> +
> +		can1: can@e6c38000 {
> +			compatible = "renesas,can-r8a77965",
> +				     "renesas,rcar-gen3-can";
> +			reg = <0 0xe6c38000 0 0x1000>;
> +			/* placeholder */
> +			status = "disabled";
> +		};
> +
>  		pwm0: pwm@e6e30000 {
>  			compatible = "renesas,pwm-r8a77965", "renesas,pwm-rcar";
>  			reg = <0 0xe6e30000 0 8>;
> 

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Simon Horman]

On Fri, Aug 17, 2018 at 02:53:37PM +0100, Kieran Bingham wrote:
> Hi Eugeniu,
> 
> Thank you for the patch,
> 
> On 12/08/18 14:31, Eugeniu Rosca wrote:
> > According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > interfaces, similar to H3, M3-W and other SoCs from the same family.
> > 
> > Add CAN placeholder nodes to avoid below DTC errors:
> > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> > 
> > These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > Fix them beforehand.
> > 
> > CAN support is inspired from below commits:
> >  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> >  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> >  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks Eugeniu and Kieran,

I have applied this for v4.20.

> > ---
> > Changes in v2:
> >  - [Kieran Bingham] Improved commit description:
> >    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
> >    - Kept the "true story" behind the patch. Just made it more clear.
> >  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
> >    (no CAN testing was done to validate the DTS configuration).
> > ---
> >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > index 486aecacb22a..4da479d3c226 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > @@ -656,6 +656,22 @@
> >  			status = "disabled";
> >  		};
> >  
> > +		can0: can@e6c30000 {
> > +			compatible = "renesas,can-r8a77965",
> > +				     "renesas,rcar-gen3-can";
> > +			reg = <0 0xe6c30000 0 0x1000>;
> > +			/* placeholder */
> > +			status = "disabled";
> > +		};
> 
> This is probably more detail than is needed for a placeholder, but it
> looks correct so I think this is fine.

Agreed.

> 
> --
> Kieran
> 
> 
> 
> > +
> > +		can1: can@e6c38000 {
> > +			compatible = "renesas,can-r8a77965",
> > +				     "renesas,rcar-gen3-can";
> > +			reg = <0 0xe6c38000 0 0x1000>;
> > +			/* placeholder */
> > +			status = "disabled";
> > +		};
> > +
> >  		pwm0: pwm@e6e30000 {
> >  			compatible = "renesas,pwm-r8a77965", "renesas,pwm-rcar";
> >  			reg = <0 0xe6e30000 0 8>;
> > 
> 

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Geert Uytterhoeven]

On Fri, Aug 17, 2018 at 3:53 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 12/08/18 14:31, Eugeniu Rosca wrote:
> > According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > interfaces, similar to H3, M3-W and other SoCs from the same family.
> >
> > Add CAN placeholder nodes to avoid below DTC errors:
> > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> >
> > These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > Fix them beforehand.
> >
> > CAN support is inspired from below commits:
> >  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> >  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> >  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> >
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
>
> > ---
> > Changes in v2:
> >  - [Kieran Bingham] Improved commit description:
> >    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
> >    - Kept the "true story" behind the patch. Just made it more clear.
> >  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
> >    (no CAN testing was done to validate the DTS configuration).
> > ---
> >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > index 486aecacb22a..4da479d3c226 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > @@ -656,6 +656,22 @@
> >                       status = "disabled";
> >               };
> >
> > +             can0: can@e6c30000 {
> > +                     compatible = "renesas,can-r8a77965",
> > +                                  "renesas,rcar-gen3-can";
> > +                     reg = <0 0xe6c30000 0 0x1000>;
> > +                     /* placeholder */
> > +                     status = "disabled";
> > +             };
>
> This is probably more detail than is needed for a placeholder, but it
> looks correct so I think this is fine.

Indeed. Adding the "compatible" properties means they're no longer
placeholders, and will be probed by the driver, possibly leading to
undefined behavior.

Hence please limit the placeholders to the absolute required minimum,
and thus drop the "compatible" and "status" properties.

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 v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Sergei Shtylyov]

On 8/23/2018 11:52 AM, Geert Uytterhoeven wrote:

>>> According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
>>> interfaces, similar to H3, M3-W and other SoCs from the same family.
>>>
>>> Add CAN placeholder nodes to avoid below DTC errors:
>>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
>>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
>>>
>>> These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
>>> Fix them beforehand.
>>>
>>> CAN support is inspired from below commits:
>>>   - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
>>>   - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
>>>   - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
>>>
>>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>>
>>> ---
>>> Changes in v2:
>>>   - [Kieran Bingham] Improved commit description:
>>>     - Referenced the newer HW manual rev1.00 instead of rev0.55E.
>>>     - Kept the "true story" behind the patch. Just made it more clear.
>>>   - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
>>>     (no CAN testing was done to validate the DTS configuration).
>>> ---
>>>   arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
>>> index 486aecacb22a..4da479d3c226 100644
>>> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
>>> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
>>> @@ -656,6 +656,22 @@
>>>                        status = "disabled";
>>>                };
>>>
>>> +             can0: can@e6c30000 {
>>> +                     compatible = "renesas,can-r8a77965",
>>> +                                  "renesas,rcar-gen3-can";
>>> +                     reg = <0 0xe6c30000 0 0x1000>;
>>> +                     /* placeholder */
>>> +                     status = "disabled";
>>> +             };
>>
>> This is probably more detail than is needed for a placeholder, but it
>> looks correct so I think this is fine.
> 
> Indeed. Adding the "compatible" properties means they're no longer
> placeholders, and will be probed by the driver, possibly leading to
> undefined behavior.

    I don't think the disabled device nodes are actually probed.

> Hence please limit the placeholders to the absolute required minimum,
> and thus drop the "compatible" and "status" properties.

    OTOH, they're not needed (yet).

> Gr{oetje,eeting}s,
> 
>                          Geert

MBR, Sergei

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Geert Uytterhoeven]

Hi Sergei,

On Thu, Aug 23, 2018 at 10:56 AM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 8/23/2018 11:52 AM, Geert Uytterhoeven wrote:
> >>> According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> >>> interfaces, similar to H3, M3-W and other SoCs from the same family.
> >>>
> >>> Add CAN placeholder nodes to avoid below DTC errors:
> >>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> >>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> >>>
> >>> These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> >>> Fix them beforehand.
> >>>
> >>> CAN support is inspired from below commits:
> >>>   - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> >>>   - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> >>>   - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> >>>
> >>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

> >>> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> >>> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> >>> @@ -656,6 +656,22 @@
> >>>                        status = "disabled";
> >>>                };
> >>>
> >>> +             can0: can@e6c30000 {
> >>> +                     compatible = "renesas,can-r8a77965",
> >>> +                                  "renesas,rcar-gen3-can";
> >>> +                     reg = <0 0xe6c30000 0 0x1000>;
> >>> +                     /* placeholder */
> >>> +                     status = "disabled";
> >>> +             };
> >>
> >> This is probably more detail than is needed for a placeholder, but it
> >> looks correct so I think this is fine.
> >
> > Indeed. Adding the "compatible" properties means they're no longer
> > placeholders, and will be probed by the driver, possibly leading to
> > undefined behavior.
>
>     I don't think the disabled device nodes are actually probed.

They will be by ulcb-kf.dtsi, after the addition of
r8a77965-m3nulcb-kf.dts, cfr.
the errors and rationale documented in the commit message.

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 v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Eugeniu Rosca]

Dear reviewers,

On Thu, Aug 23, 2018 at 11:01:46AM +0200, Geert Uytterhoeven wrote:
> Hi Sergei,
> 
> On Thu, Aug 23, 2018 at 10:56 AM Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
> > On 8/23/2018 11:52 AM, Geert Uytterhoeven wrote:
> > >>> According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > >>> interfaces, similar to H3, M3-W and other SoCs from the same family.
> > >>>
> > >>> Add CAN placeholder nodes to avoid below DTC errors:
> > >>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > >>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> > >>>
> > >>> These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > >>> Fix them beforehand.
> > >>>
> > >>> CAN support is inspired from below commits:
> > >>>   - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> > >>>   - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> > >>>   - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > >>>
> > >>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> > >>> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > >>> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > >>> @@ -656,6 +656,22 @@
> > >>>                        status = "disabled";
> > >>>                };
> > >>>
> > >>> +             can0: can@e6c30000 {
> > >>> +                     compatible = "renesas,can-r8a77965",
> > >>> +                                  "renesas,rcar-gen3-can";
> > >>> +                     reg = <0 0xe6c30000 0 0x1000>;
> > >>> +                     /* placeholder */
> > >>> +                     status = "disabled";
> > >>> +             };
> > >>
> > >> This is probably more detail than is needed for a placeholder, but it
> > >> looks correct so I think this is fine.
> > >
> > > Indeed. Adding the "compatible" properties means they're no longer
> > > placeholders, and will be probed by the driver, possibly leading to
> > > undefined behavior.
> >
> >     I don't think the disabled device nodes are actually probed.
> 
> They will be by ulcb-kf.dtsi, after the addition of
> r8a77965-m3nulcb-kf.dts, cfr.
> the errors and rationale documented in the commit message.

I took some time to examine the "52. Controller Area Network Interface
(CAN interface)" chapter of HW SoC manual rev1.00 in detail and there is
no difference mentioned between the SoCs (H3, M3-W, M3-N, D3, E3) which
implement the two CAN (non-FD) interfaces. This is confirmed by the
perfectly symmetrical can{0,1} configuration present in the H3,
M3-W and D3 device tree sources:

$ git grep -l can0 -- arch/arm64/boot/dts/renesas/r8*dtsi
arch/arm64/boot/dts/renesas/r8a7795.dtsi
arch/arm64/boot/dts/renesas/r8a7796.dtsi
arch/arm64/boot/dts/renesas/r8a77995.dtsi

So, to be honest, in my opinion, besides consuming time arguing about
what a placeholder DTS node is (btw, commits [1] and [2] do include a
compatible string while adding a "placeholder" node), we also force
users to 'git blame' multiple times to reconstruct the history of CAN
controller nodes on M3-N, while for H3, M3-W and D3 a single commit was
enough to add the functionality.

That said, I don't see any dmesg differences on M3NULCB between having
and not having the compatible string in the can{0,1} nodes.

> Hence please limit the placeholders to the absolute required minimum,
> and thus drop the "compatible" and "status" properties.

My understanding is that the lack of status is equivalent with
'status = "okay"' (i.e. enable the node), so I don't really see why
'status = "disabled"' should hurt for a placeholder, especially seeing
a high number of commits [3] using 'status = "disabled"' by default. At
least I ask for an explanation regarding this last part before
incrementing the version of this patch. TIA.

[1] fae3a9f023b7 ("ARM: dts: dra7: Add ti,secure-ram node to ocmcram1 node")
[2] 162669876bbe ("ARM: dts: sun9i: Switch to the AC100 RTC clock outputs for osc32k")
[3] git blame arch/arm64/boot/dts/renesas/r8a7795.dtsi | grep disabled | awk '{print $1}' | sort -u | wc -l
    22

> 
> Gr{oetje,eeting}s,
> 
>                         Geert

Thanks,
Eugeniu.

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Kieran Bingham]

Hi Eugeniu,

On 23/08/18 18:14, Eugeniu Rosca wrote:
> Dear reviewers,
> 
> On Thu, Aug 23, 2018 at 11:01:46AM +0200, Geert Uytterhoeven wrote:
>> Hi Sergei,
>>
>> On Thu, Aug 23, 2018 at 10:56 AM Sergei Shtylyov
>> <sergei.shtylyov@cogentembedded.com> wrote:
>>> On 8/23/2018 11:52 AM, Geert Uytterhoeven wrote:
>>>>>> According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
>>>>>> interfaces, similar to H3, M3-W and other SoCs from the same family.
>>>>>>
>>>>>> Add CAN placeholder nodes to avoid below DTC errors:
>>>>>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
>>>>>> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
>>>>>>
>>>>>> These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
>>>>>> Fix them beforehand.
>>>>>>
>>>>>> CAN support is inspired from below commits:
>>>>>>   - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
>>>>>>   - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
>>>>>>   - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
>>>>>>
>>>>>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>>
>>>>>> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
>>>>>> @@ -656,6 +656,22 @@
>>>>>>                        status = "disabled";
>>>>>>                };
>>>>>>
>>>>>> +             can0: can@e6c30000 {
>>>>>> +                     compatible = "renesas,can-r8a77965",
>>>>>> +                                  "renesas,rcar-gen3-can";
>>>>>> +                     reg = <0 0xe6c30000 0 0x1000>;
>>>>>> +                     /* placeholder */
>>>>>> +                     status = "disabled";
>>>>>> +             };
>>>>>
>>>>> This is probably more detail than is needed for a placeholder, but it
>>>>> looks correct so I think this is fine.
>>>>
>>>> Indeed. Adding the "compatible" properties means they're no longer
>>>> placeholders, and will be probed by the driver, possibly leading to
>>>> undefined behavior.
>>>
>>>     I don't think the disabled device nodes are actually probed.
>>
>> They will be by ulcb-kf.dtsi, after the addition of
>> r8a77965-m3nulcb-kf.dts, cfr.
>> the errors and rationale documented in the commit message.
> 
> I took some time to examine the "52. Controller Area Network Interface
> (CAN interface)" chapter of HW SoC manual rev1.00 in detail and there is
> no difference mentioned between the SoCs (H3, M3-W, M3-N, D3, E3) which
> implement the two CAN (non-FD) interfaces. This is confirmed by the
> perfectly symmetrical can{0,1} configuration present in the H3,
> M3-W and D3 device tree sources:
> 
> $ git grep -l can0 -- arch/arm64/boot/dts/renesas/r8*dtsi
> arch/arm64/boot/dts/renesas/r8a7795.dtsi
> arch/arm64/boot/dts/renesas/r8a7796.dtsi
> arch/arm64/boot/dts/renesas/r8a77995.dtsi


The problem is, until it's tested - we don't actually know it works.
Even 'identical' chips can have different changes between the revisions.



> So, to be honest, in my opinion, besides consuming time arguing about
> what a placeholder DTS node is (btw, commits [1] and [2] do include a
> compatible string while adding a "placeholder" node),

[1] is a special case. It's not actually going to probe a driver.

sources/linux$ git grep secure-ram
arch/arm/boot/dts/dra7.dtsi:  compatible = "ti,secure-ram";

<there is no driver which will match on this compatible>


[2] is also a different case:

You can see in the patch that the 'placeholder' is used in the same
commit by sun9i-a80-cubieboard4.dts and sun9i-a80-optimus.dts



> we also force
> users to 'git blame' multiple times to reconstruct the history of CAN
> controller nodes on M3-N, while for H3, M3-W and D3 a single commit was
> enough to add the functionality.
> 
> That said, I don't see any dmesg differences on M3NULCB between having
> and not having the compatible string in the can{0,1} nodes.
> 
>> Hence please limit the placeholders to the absolute required minimum,
>> and thus drop the "compatible" and "status" properties.
> 
> My understanding is that the lack of status is equivalent with
> 'status = "okay"' (i.e. enable the node), so I don't really see why
> 'status = "disabled"' should hurt for a placeholder, especially seeing
> a high number of commits [3] using 'status = "disabled"' by default. 

It's not so much the 'status' field that hurts in this patch, but the
compatible matching.

The file r8a7795.dtsi is a SoC level description. It tries to describe
all the features provided by that SoC. But all of those features may not
be brought out on the "board", and so many are disabled by default.

 - Essentially 'status = "disabled";'
    is like saying - This node is good - but don't use it.


It's then up to the board file (Salvator-XS.dtb, ULCB.dtb
YourBoardHere.dtb ...) to enable the features that are available.

That's the case here in the KingFisher board.

The ulcb-kf.dtsi will override the CAN nodes and set 'status = Okay' to
enable the devices available on the KF board:

> &can0 {
>         pinctrl-0 = <&can0_pins>;
>         pinctrl-names = "default";
>         status = "okay";
> };


So - once that file is included (even though you have set 'status =
'disabled'), the DTB asserts that the can0 is connected, available, and
'okay' to be probed.


For an example of adding 'lots' of placeholders take a look at
df863d6f95f5 ("arm64: dts: renesas: initial R8A77965 SoC device tree")


and a patch which then populates a placeholder when it's ready:
2f2c71bfc8c5 ("arm64: dts: renesas: r8a77965: Populate the DU instance
placeholder")




Now we get political rather than technical ...

If you are adding the DTB node which implements this (by adding the full
node in r8a77965.dtsi), you should be able to assert that this is
functional. Either by testing it yourself, or by finding someone to
submit a Tested-by: <xxx@yyy.com> tag.

The alternative, is to provide a placeholder which is set as
non-functional - and will not probe. <which you've mostly done>

It's just that by providing the compatible strings in your placeholder,
the status = "okay" /will/ allow the driver framework to probe this device.

Then we are into undefined behaviour because we are saying that the
device is ready to probe - but without fully defining the required
properties.

> At
> least I ask for an explanation regarding this last part before
> incrementing the version of this patch. TIA.

I hope that's made something clearer in a murky world.

Welcome to open source :D ... I appreciate your frustration on
re-spinning patches that are not your intended target or goal. I've
faced similar blocking situations in my time.

It's all part of the goal of striving towards 'perfect-as-can-be'
upstream code I guess.

> [1] fae3a9f023b7 ("ARM: dts: dra7: Add ti,secure-ram node to ocmcram1 node")
> [2] 162669876bbe ("ARM: dts: sun9i: Switch to the AC100 RTC clock outputs for osc32k")
> [3] git blame arch/arm64/boot/dts/renesas/r8a7795.dtsi | grep disabled | awk '{print $1}' | sort -u | wc -l
>     22
> 
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
> 
> Thanks,
> Eugeniu.


--
Kieran

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Eugeniu Rosca]

Hi Kieran,

I appreciate the detailed reply zooming in into many aspects (both
technical and related to process/workflow) of contributing/reviewing
patches. I take it as is. I could elaborate on specific parts of it,
like applying the "undefined behavior" term (which comes from the world
of compilers) to the software we write. This doesn't sound right to me,
since our software is guided by our own requirements and one of this
requirements is (should be) predictable and safe behavior given some
junky/incomplete DTS configuration. If any bugs exist dealing with
nonsense configuration, IMHO they should be fixed in the driver. Since
my purpose is simply seeing the patches in upstream the way maintainers
like them best, I will put little to no time discussing this and will
mainly focus on submitting any changes requested by the reviewers.

Thanks,
Eugeniu.

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Simon Horman]

On Thu, Aug 23, 2018 at 10:52:09AM +0200, Geert Uytterhoeven wrote:
> On Fri, Aug 17, 2018 at 3:53 PM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> > On 12/08/18 14:31, Eugeniu Rosca wrote:
> > > According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > > interfaces, similar to H3, M3-W and other SoCs from the same family.
> > >
> > > Add CAN placeholder nodes to avoid below DTC errors:
> > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> > >
> > > These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > > Fix them beforehand.
> > >
> > > CAN support is inspired from below commits:
> > >  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> > >  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> > >  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > >
> > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> >
> > > ---
> > > Changes in v2:
> > >  - [Kieran Bingham] Improved commit description:
> > >    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
> > >    - Kept the "true story" behind the patch. Just made it more clear.
> > >  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
> > >    (no CAN testing was done to validate the DTS configuration).
> > > ---
> > >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > index 486aecacb22a..4da479d3c226 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > @@ -656,6 +656,22 @@
> > >                       status = "disabled";
> > >               };
> > >
> > > +             can0: can@e6c30000 {
> > > +                     compatible = "renesas,can-r8a77965",
> > > +                                  "renesas,rcar-gen3-can";
> > > +                     reg = <0 0xe6c30000 0 0x1000>;
> > > +                     /* placeholder */
> > > +                     status = "disabled";
> > > +             };
> >
> > This is probably more detail than is needed for a placeholder, but it
> > looks correct so I think this is fine.
> 
> Indeed. Adding the "compatible" properties means they're no longer
> placeholders, and will be probed by the driver, possibly leading to
> undefined behavior.
> 
> Hence please limit the placeholders to the absolute required minimum,
> and thus drop the "compatible" and "status" properties.

Agreed, I will update the patch accordingly.

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Eugeniu Rosca]

Hi Simon, hi Geert,

On Mon, Aug 27, 2018 at 02:44:47PM +0200, Simon Horman wrote:
> On Thu, Aug 23, 2018 at 10:52:09AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Aug 17, 2018 at 3:53 PM Kieran Bingham
> > <kieran.bingham+renesas@ideasonboard.com> wrote:
> > > On 12/08/18 14:31, Eugeniu Rosca wrote:
> > > > According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > > > interfaces, similar to H3, M3-W and other SoCs from the same family.
> > > >
> > > > Add CAN placeholder nodes to avoid below DTC errors:
> > > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> > > >
> > > > These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > > > Fix them beforehand.
> > > >
> > > > CAN support is inspired from below commits:
> > > >  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> > > >  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> > > >  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > > >
> > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >
> > >
> > > > ---
> > > > Changes in v2:
> > > >  - [Kieran Bingham] Improved commit description:
> > > >    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
> > > >    - Kept the "true story" behind the patch. Just made it more clear.
> > > >  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
> > > >    (no CAN testing was done to validate the DTS configuration).
> > > > ---
> > > >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > index 486aecacb22a..4da479d3c226 100644
> > > > --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > @@ -656,6 +656,22 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             can0: can@e6c30000 {
> > > > +                     compatible = "renesas,can-r8a77965",
> > > > +                                  "renesas,rcar-gen3-can";
> > > > +                     reg = <0 0xe6c30000 0 0x1000>;
> > > > +                     /* placeholder */
> > > > +                     status = "disabled";
> > > > +             };
> > >
> > > This is probably more detail than is needed for a placeholder, but it
> > > looks correct so I think this is fine.
> > 
> > Indeed. Adding the "compatible" properties means they're no longer
> > placeholders, and will be probed by the driver, possibly leading to
> > undefined behavior.
> > 
> > Hence please limit the placeholders to the absolute required minimum,
> > and thus drop the "compatible" and "status" properties.
> 
> Agreed, I will update the patch accordingly.

My understanding is that Simon will drop the compatible strings and no
action is needed from my end. Let me know otherwise.

Thanks,
Eugeniu.

Re: [PATCH v2 5/7] arm64: dts: renesas: r8a77965: Add CAN{0,1} placeholder nodes

[Simon Horman]

On Mon, Aug 27, 2018 at 09:28:09PM +0200, Eugeniu Rosca wrote:
> Hi Simon, hi Geert,
> 
> On Mon, Aug 27, 2018 at 02:44:47PM +0200, Simon Horman wrote:
> > On Thu, Aug 23, 2018 at 10:52:09AM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Aug 17, 2018 at 3:53 PM Kieran Bingham
> > > <kieran.bingham+renesas@ideasonboard.com> wrote:
> > > > On 12/08/18 14:31, Eugeniu Rosca wrote:
> > > > > According to R-Car Gen3 HW manual rev1.00, R-Car M3-N has two CAN
> > > > > interfaces, similar to H3, M3-W and other SoCs from the same family.
> > > > >
> > > > > Add CAN placeholder nodes to avoid below DTC errors:
> > > > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:19.1-6 Label or path can0 not found
> > > > > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:25.1-6 Label or path can1 not found
> > > > >
> > > > > These errors occur *after* the addition of r8a77965-m3nulcb-kf.dts.
> > > > > Fix them beforehand.
> > > > >
> > > > > CAN support is inspired from below commits:
> > > > >  - v4.7 commit 308b7e4ba62e ("arm64: dts: r8a7795: Add CAN support")
> > > > >  - v4.11 commit 909c16252415 ("arm64: dts: r8a7796: Add CAN support")
> > > > >  - v4.12 commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > > > >
> > > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > >
> > > >
> > > > > ---
> > > > > Changes in v2:
> > > > >  - [Kieran Bingham] Improved commit description:
> > > > >    - Referenced the newer HW manual rev1.00 instead of rev0.55E.
> > > > >    - Kept the "true story" behind the patch. Just made it more clear.
> > > > >  - [Geert Uytterhoeven] Replaced CAN0 and CAN1 nodes with placeholders
> > > > >    (no CAN testing was done to validate the DTS configuration).
> > > > > ---
> > > > >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > > index 486aecacb22a..4da479d3c226 100644
> > > > > --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > > +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> > > > > @@ -656,6 +656,22 @@
> > > > >                       status = "disabled";
> > > > >               };
> > > > >
> > > > > +             can0: can@e6c30000 {
> > > > > +                     compatible = "renesas,can-r8a77965",
> > > > > +                                  "renesas,rcar-gen3-can";
> > > > > +                     reg = <0 0xe6c30000 0 0x1000>;
> > > > > +                     /* placeholder */
> > > > > +                     status = "disabled";
> > > > > +             };
> > > >
> > > > This is probably more detail than is needed for a placeholder, but it
> > > > looks correct so I think this is fine.
> > > 
> > > Indeed. Adding the "compatible" properties means they're no longer
> > > placeholders, and will be probed by the driver, possibly leading to
> > > undefined behavior.
> > > 
> > > Hence please limit the placeholders to the absolute required minimum,
> > > and thus drop the "compatible" and "status" properties.
> > 
> > Agreed, I will update the patch accordingly.
> 
> My understanding is that Simon will drop the compatible strings and no
> action is needed from my end. Let me know otherwise.

Your understanding is correct.

[PATCH v2 6/7] arm64: dts: renesas: r8a77965: Add HSCIF0 device node

[Eugeniu Rosca]

Fix below DTC error:
Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:36.1-8 Label or path hscif0 not found

The DTC error occurs *after* the addition of r8a77965-m3nulcb-kf.dts.
Fix it beforehand.

Inspired from v4.12-rc1 commits:
 - commit 68cd16107260 ("arm64: dts: r8a7796 dtsi: Add all HSCIF nodes")
 - commit 6d50bb893504 ("arm64: dts: r8a7796: Enable HSCIF DMA")
 - commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - Slightly improved the commit description.
 - Note that this commit could be replaced by
   https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git/commit/?h=b8e3c8e17611
---
 arch/arm64/boot/dts/renesas/r8a77965.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
index 4da479d3c226..d04a8b671cc2 100644
--- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -451,6 +451,24 @@
 			status = "disabled";
 		};
 
+		hscif0: serial@e6540000 {
+			compatible = "renesas,hscif-r8a77965",
+				     "renesas,rcar-gen3-hscif",
+				     "renesas,hscif";
+			reg = <0 0xe6540000 0 0x60>;
+			interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 520>,
+				 <&cpg CPG_CORE R8A77965_CLK_S3D1>,
+				 <&scif_clk>;
+			clock-names = "fck", "brg_int", "scif_clk";
+			dmas = <&dmac1 0x31>, <&dmac1 0x30>,
+			       <&dmac2 0x31>, <&dmac2 0x30>;
+			dma-names = "tx", "rx", "tx", "rx";
+			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
+			resets = <&cpg 520>;
+			status = "disabled";
+		};
+
 		hsusb: usb@e6590000 {
 			compatible = "renesas,usbhs-r8a7796",
 				     "renesas,rcar-gen3-usbhs";
-- 
2.18.0

Re: [PATCH v2 6/7] arm64: dts: renesas: r8a77965: Add HSCIF0 device node

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:48PM +0200, Eugeniu Rosca wrote:
> Fix below DTC error:
> Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:36.1-8 Label or path hscif0 not found
> 
> The DTC error occurs *after* the addition of r8a77965-m3nulcb-kf.dts.
> Fix it beforehand.
> 
> Inspired from v4.12-rc1 commits:
>  - commit 68cd16107260 ("arm64: dts: r8a7796 dtsi: Add all HSCIF nodes")
>  - commit 6d50bb893504 ("arm64: dts: r8a7796: Enable HSCIF DMA")
>  - commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> Changes in v2:
>  - Slightly improved the commit description.
>  - Note that this commit could be replaced by
>    https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git/commit/?h=b8e3c8e17611

That commit has been accepted for upstream so it should be used.

> ---
>  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> index 4da479d3c226..d04a8b671cc2 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> @@ -451,6 +451,24 @@
>  			status = "disabled";
>  		};
>  
> +		hscif0: serial@e6540000 {
> +			compatible = "renesas,hscif-r8a77965",
> +				     "renesas,rcar-gen3-hscif",
> +				     "renesas,hscif";
> +			reg = <0 0xe6540000 0 0x60>;
> +			interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 520>,
> +				 <&cpg CPG_CORE R8A77965_CLK_S3D1>,
> +				 <&scif_clk>;
> +			clock-names = "fck", "brg_int", "scif_clk";
> +			dmas = <&dmac1 0x31>, <&dmac1 0x30>,
> +			       <&dmac2 0x31>, <&dmac2 0x30>;
> +			dma-names = "tx", "rx", "tx", "rx";
> +			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
> +			resets = <&cpg 520>;
> +			status = "disabled";
> +		};
> +
>  		hsusb: usb@e6590000 {
>  			compatible = "renesas,usbhs-r8a7796",
>  				     "renesas,rcar-gen3-usbhs";
> -- 
> 2.18.0
> 

Re: [PATCH v2 6/7] arm64: dts: renesas: r8a77965: Add HSCIF0 device node

[Eugeniu Rosca]

Hi Simon,

On Wed, Aug 22, 2018 at 01:12:47PM +0200, Simon Horman wrote:
> On Sun, Aug 12, 2018 at 03:31:48PM +0200, Eugeniu Rosca wrote:
> > Fix below DTC error:
> > Error: arch/arm64/boot/dts/renesas/ulcb-kf.dtsi:36.1-8 Label or path hscif0 not found
> > 
> > The DTC error occurs *after* the addition of r8a77965-m3nulcb-kf.dts.
> > Fix it beforehand.
> > 
> > Inspired from v4.12-rc1 commits:
> >  - commit 68cd16107260 ("arm64: dts: r8a7796 dtsi: Add all HSCIF nodes")
> >  - commit 6d50bb893504 ("arm64: dts: r8a7796: Enable HSCIF DMA")
> >  - commit bec0948e810f ("arm64: dts: r8a7796: Add reset control properties")
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> > Changes in v2:
> >  - Slightly improved the commit description.
> >  - Note that this commit could be replaced by
> >    https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git/commit/?h=b8e3c8e17611
> 
> That commit has been accepted for upstream so it should be used.

Thanks for the explanation. No further questions/concerns.

Best regards,
Eugeniu.

[PATCH v2 7/7] arm64: dts: renesas: r8a77965: m3nulcb-kf: Initial device tree

[Eugeniu Rosca]

This is based on the existing KF device tree sources:
$ ls -1 arch/arm64/boot/dts/renesas/*-kf.dts
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dts
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dts
arch/arm64/boot/dts/renesas/r8a7796-m3ulcb-kf.dts

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - [Jacopo Mondi] Removed redundant license text.
 - [Simon Horman]
   - Renamed DTS 's/r8a77965-ulcb-kf.dts/r8a77965-m3nulcb-kf.dts/'
   - Renamed board name 's/M3-N ULCB/M3NULCB/'
   - Renamed compatible string 's/renesas,ulcb/renesas,m3nulcb/'
   - Adapted Makefile and commit summary line 's/ulcb-kf/m3nulcb-kf/'
 - Documented the source of inspiration in the commit description.
---
 arch/arm64/boot/dts/renesas/Makefile             |  1 +
 .../boot/dts/renesas/r8a77965-m3nulcb-kf.dts     | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/r8a77965-m3nulcb-kf.dts

diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index eb158d1f90e9..a8ce6594342d 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -9,6 +9,7 @@ dtb-$(CONFIG_ARCH_R8A7796) += r8a7796-m3ulcb-kf.dtb
 dtb-$(CONFIG_ARCH_R8A7796) += r8a7796-salvator-xs.dtb
 dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-salvator-x.dtb r8a77965-salvator-xs.dtb
 dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-m3nulcb.dtb
+dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-m3nulcb-kf.dtb
 dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-eagle.dtb r8a77970-v3msk.dtb
 dtb-$(CONFIG_ARCH_R8A77980) += r8a77980-condor.dtb r8a77980-v3hsk.dtb
 dtb-$(CONFIG_ARCH_R8A77990) += r8a77990-ebisu.dtb
diff --git a/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb-kf.dts b/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb-kf.dts
new file mode 100644
index 000000000000..dadad97051b9
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a77965-m3nulcb-kf.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the M3NULCB Kingfisher board
+ *
+ * Copyright (C) 2018 Renesas Electronics Corp.
+ * Copyright (C) 2018 Cogent Embedded, Inc.
+ */
+
+#include "r8a77965-m3nulcb.dts"
+#include "ulcb-kf.dtsi"
+
+/ {
+	model = "Renesas M3NULCB Kingfisher board based on r8a77965";
+	compatible = "shimafuji,kingfisher", "renesas,m3nulcb",
+		     "renesas,r8a77965";
+};
-- 
2.18.0

Re: [PATCH v2 7/7] arm64: dts: renesas: r8a77965: m3nulcb-kf: Initial device tree

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:49PM +0200, Eugeniu Rosca wrote:
> This is based on the existing KF device tree sources:
> $ ls -1 arch/arm64/boot/dts/renesas/*-kf.dts
> arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dts
> arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dts
> arch/arm64/boot/dts/renesas/r8a7796-m3ulcb-kf.dts
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks,

This looks fine to me but I will wait to see if there are other reviews
before applying.

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

Re: [PATCH v2 7/7] arm64: dts: renesas: r8a77965: m3nulcb-kf: Initial device tree

[Simon Horman]

On Wed, Aug 22, 2018 at 01:14:09PM +0200, Simon Horman wrote:
> On Sun, Aug 12, 2018 at 03:31:49PM +0200, Eugeniu Rosca wrote:
> > This is based on the existing KF device tree sources:
> > $ ls -1 arch/arm64/boot/dts/renesas/*-kf.dts
> > arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dts
> > arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dts
> > arch/arm64/boot/dts/renesas/r8a7796-m3ulcb-kf.dts
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Thanks,
> 
> This looks fine to me but I will wait to see if there are other reviews
> before applying.

Thanks again, applied for v4.20.

I believe that this entire series has now been applied
or at least all of the portions to be merged via my
renesas tree. Please let me know if I am mistaken.

Re: [PATCH v2 1/7] dt-bindings: arm: Document Renesas R-Car M3-N-based ULCB board

[Simon Horman]

On Sun, Aug 12, 2018 at 03:31:43PM +0200, Eugeniu Rosca wrote:
> In harmony with ATF and U-Boot outputs [1] and [2], the new board is
> based on M3-N revision ES1.1 and the amount of memory present on SiP
> is 2GiB, contiguously addressed.
> 
> The amount of RAM is mentioned based on the assumption that it is
> encoded in the board id/string. There is some evidence supporting this
> in form of last-digit-mismatch between two R-Car H3 ES2.0 ULCB board
> ids, one with 4GiB and one with 8GiB of RAM (see [3]).
> 
> [1] BL2: R-Car Gen3 Initial Program Loader(CA57) Rev.1.0.21
>     BL2: PRR is R-Car M3N Ver.1.1
> 
> [2] U-Boot 2015.04-00295-*
>     CPU: Renesas Electronics R8A77965 rev 1.1
>     ---8<----
>     DRAM:  1.9 GiB
>     Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB
>     ---8<----
> 
> [3] https://patchwork.kernel.org/patch/10555957/#22169325
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks,

This looks fine to me but I will wait to see if there are other reviews
before applying.

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

Re: [PATCH v2 1/7] dt-bindings: arm: Document Renesas R-Car M3-N-based ULCB board

[Simon Horman]

On Fri, Aug 17, 2018 at 12:08:24PM +0200, Simon Horman wrote:
> On Sun, Aug 12, 2018 at 03:31:43PM +0200, Eugeniu Rosca wrote:
> > In harmony with ATF and U-Boot outputs [1] and [2], the new board is
> > based on M3-N revision ES1.1 and the amount of memory present on SiP
> > is 2GiB, contiguously addressed.
> > 
> > The amount of RAM is mentioned based on the assumption that it is
> > encoded in the board id/string. There is some evidence supporting this
> > in form of last-digit-mismatch between two R-Car H3 ES2.0 ULCB board
> > ids, one with 4GiB and one with 8GiB of RAM (see [3]).
> > 
> > [1] BL2: R-Car Gen3 Initial Program Loader(CA57) Rev.1.0.21
> >     BL2: PRR is R-Car M3N Ver.1.1
> > 
> > [2] U-Boot 2015.04-00295-*
> >     CPU: Renesas Electronics R8A77965 rev 1.1
> >     ---8<----
> >     DRAM:  1.9 GiB
> >     Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB
> >     ---8<----
> > 
> > [3] https://patchwork.kernel.org/patch/10555957/#22169325
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Thanks,
> 
> This looks fine to me but I will wait to see if there are other reviews
> before applying.
> 
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Thanks again, applied for v4.20.