| Message ID | 20180812133149.7710-5-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 92bc66bfce99cd7d5b66cb1086812a32300c5705 |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [v2,1/7] dt-bindings: arm: Document Renesas R-Car M3-N-based ULCB board | expand |
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>; >
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>; > > >
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
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
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
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.
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
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.
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.
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.
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.
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>;
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(+)