Re: [PATCH v11 19/19] h8300: devicetree source
From: Mark Rutland <mark.rutland@arm.com>
Date: 2015-05-08 16:50:14
Also in:
linux-arch, lkml
Hi,
quoted hunk ↗ jump to hunk
+++ b/Documentation/devicetree/bindings/h8300/cpu.txt@@ -0,0 +1,17 @@ +* H8/300 CPU bindings + +Required properties: + +- compatible: Compatible property value should be "renesas,h8300". +- reg: Contains CPU index.
What does the "CPU index" correspond to physically on the CPU? Can h8300 support SMP?
+- clock-frequency: Contains the clock frequency for CPU, in Hz.
Is this strictly necessary?
+- renesas,bus-width: Contain the memory bus width.
What's this needed for? [...]
quoted hunk ↗ jump to hunk
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,h8300h-intc.txt@@ -0,0 +1,20 @@ +* H8/300H Interrupt controller + +Required properties: + +- compatible: has to be "renesas,h8300h-intc", "renesas,h8300-intc" as fallback. +- #interrupt-cells: has to be <1>: an interrupt index and flags, as defined in + interrupts.txt in this directory
Surely you need two cells to encode index and flags?
+
+Optional properties:
+
+- any properties, listed in interrupts.txt, and any standard resource allocation
+ properties
+
+Example:
+
+ h8intc: intc@0 {Nit: call this "interrupt-controller" rather than "ntc". Without a reg you shouldn't have a unit-address (the '@0' part). How does the CPU communicate with this controller? I assume it's not MMIO. These comments also apply to "renesas,h8s-intc".
quoted hunk ↗ jump to hunk
diff --git a/arch/h8300/boot/dts/edosk2674.dts b/arch/h8300/boot/dts/edosk2674.dts new file mode 100644 index 0000000..60e73b9 --- /dev/null +++ b/arch/h8300/boot/dts/edosk2674.dts@@ -0,0 +1,109 @@ +#include <dt-bindings/clock/renesas,8bit-timer.h> + +/dts-v1/; +/ { + compatible = "renesas,edosk2674"; + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <&h8intc>; + + chosen { + bootargs = "console=ttySC2,38400"; + };
It would be great if you could use stdout-path from the start rather than passing console= in bootargs. That makes things far less fragile w.r.t. physical vs logical naming, and keeps console and earlycon in sync. It also means that a user can replace bootargs and still expect the console to work by default (unless overridden explicitly).
+ aliases {
+ serial0 = &sci0;
+ serial1 = &sci1;
+ serial2 = &sci2;
+ };
+
+ clocks {
+ ranges;
+ #address-cells = <1>;
+ #size-cells = <1>;Please get rid of the clocks container node and place the clocks directly under the root node. There's nothing magic about a /clocks node, and it's not been listed as a bus of any sort.
+ pllclk: pllclk {
+ compatible = "renesas,h8s2678-pll-clock";
+ clocks = <&xclk>;
+ #clock-cells = <0>;
+ reg = <0xfee03b 2>, <0xfee045 2>;
+ };
+ cclk: cclk {
+ compatible = "renesas,h8300-div-clock";
+ clocks = <&pllclk>;
+ #clock-cells = <0>;
+ reg = <0xfee03b 2>;
+ renesas,width = <3>;
+ };Are there existing bindings for these? I didn't see any as part of the portion of the series I was Cc'd for.
+ memory@0 {Nit: the unit-address should math the address in the reg entry (here it should be 400000 rather than 0). [...]
+ chosen {
+ bootargs = "earlyprintk=h8300-sim console=ttySC0";
+ };If you implement earlycon you'd only need a single stdout-path entry here, which would make this much nicer. Thanks, Mark.