Patchwork [16/25] PPC: e500: dt: create serial nodes dynamically

login
register
mail settings
Submitter Alexander Graf
Date May 30, 2012, 11 a.m.
Message ID <1338375646-15064-17-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/161946/
State New
Headers show

Comments

Alexander Graf - May 30, 2012, 11 a.m.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppce500_mpc8544ds.c |   35 +++++++++++++++++++++++++++++++++++
 pc-bios/mpc8544ds.dts  |   26 --------------------------
 2 files changed, 35 insertions(+), 26 deletions(-)
Scott Wood - May 31, 2012, 10:02 p.m.
On 05/30/2012 06:00 AM, Alexander Graf wrote:
> @@ -196,6 +199,38 @@ static int mpc8544_load_device_tree(CPUPPCState *env,
>      /* XXX should contain a reasonable value */
>      qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>  
> +    /*
> +     * We have to generate ser1 first, because Linux takes the first
> +     * device it finds in the dt as serial output device. And we generate
> +     * devices in reverse order to the dt.
> +     */
> +    sprintf(ser1, "%s/serial@%x", soc,
> +            MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE);
> +    qemu_devtree_add_subnode(fdt, ser1);
> +    qemu_devtree_setprop_string(fdt, ser1, "device_type", "serial");
> +    qemu_devtree_setprop_string(fdt, ser1, "compatible", "ns16550");
> +    qemu_devtree_setprop_cell2(fdt, ser1, "reg", MPC8544_SERIAL1_REGS_BASE -
> +                               MPC8544_CCSRBAR_BASE, 0x100);
> +    qemu_devtree_setprop_cell(fdt, ser1, "cell-index", 1);
> +    qemu_devtree_setprop_cell(fdt, ser1, "clock-frequency", 0);
> +    qemu_devtree_setprop_cell2(fdt, ser1, "interrupts", 42, 2);
> +    qemu_devtree_setprop_phandle(fdt, ser1, "interrupt-parent", mpic);
> +    qemu_devtree_setprop_string(fdt, "/aliases", "serial1", ser1);

Please put this somewhere it won't have to be duplicated for every board
-- preferably in the serial device code itself.

-Scott
Alexander Graf - May 31, 2012, 10:17 p.m.
On 01.06.2012, at 00:02, Scott Wood wrote:

> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>> @@ -196,6 +199,38 @@ static int mpc8544_load_device_tree(CPUPPCState *env,
>>     /* XXX should contain a reasonable value */
>>     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>> 
>> +    /*
>> +     * We have to generate ser1 first, because Linux takes the first
>> +     * device it finds in the dt as serial output device. And we generate
>> +     * devices in reverse order to the dt.
>> +     */
>> +    sprintf(ser1, "%s/serial@%x", soc,
>> +            MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE);
>> +    qemu_devtree_add_subnode(fdt, ser1);
>> +    qemu_devtree_setprop_string(fdt, ser1, "device_type", "serial");
>> +    qemu_devtree_setprop_string(fdt, ser1, "compatible", "ns16550");
>> +    qemu_devtree_setprop_cell2(fdt, ser1, "reg", MPC8544_SERIAL1_REGS_BASE -
>> +                               MPC8544_CCSRBAR_BASE, 0x100);
>> +    qemu_devtree_setprop_cell(fdt, ser1, "cell-index", 1);
>> +    qemu_devtree_setprop_cell(fdt, ser1, "clock-frequency", 0);
>> +    qemu_devtree_setprop_cell2(fdt, ser1, "interrupts", 42, 2);
>> +    qemu_devtree_setprop_phandle(fdt, ser1, "interrupt-parent", mpic);
>> +    qemu_devtree_setprop_string(fdt, "/aliases", "serial1", ser1);
> 
> Please put this somewhere it won't have to be duplicated for every board
> -- preferably in the serial device code itself.

Yeah, I talked to Anthony about that and he didn't like the idea of devices knowing about their device tree specifics. Serial is a good candidate to understand why. A serial device shouldn't know if it's device 0 or device 1. It's just a plain device. The semantics on the port go with the board.

That said, we should probably refactor this as a generic helper, so every board that implements a serial port can just call a common helper function. I'd do that with whatever board gets converted next - probably bamboo :).


Alex
Scott Wood - May 31, 2012, 10:22 p.m.
On 05/31/2012 05:17 PM, Alexander Graf wrote:
> 
> On 01.06.2012, at 00:02, Scott Wood wrote:
>> Please put this somewhere it won't have to be duplicated for every board
>> -- preferably in the serial device code itself.
> 
> Yeah, I talked to Anthony about that and he didn't like the idea of
> devices knowing about their device tree specifics. Serial is a good
> candidate to understand why. A serial device shouldn't know if it's
> device 0 or device 1. It's just a plain device. The semantics on the
> port go with the board.

Well, cell-index should probably just be dropped.  The board code (or at
least code for the SoC family) should determine the alias name, but it
shouldn't have to take care of everything.

Consider more complicated devices with various properties describing
attributes of the device itself, that have to match what's actually
implemented.  If there is anything that is board knowledge, provide a
way for the board to add it as a supplement.

-Scott
Alexander Graf - May 31, 2012, 10:38 p.m.
On 01.06.2012, at 00:22, Scott Wood wrote:

> On 05/31/2012 05:17 PM, Alexander Graf wrote:
>> 
>> On 01.06.2012, at 00:02, Scott Wood wrote:
>>> Please put this somewhere it won't have to be duplicated for every board
>>> -- preferably in the serial device code itself.
>> 
>> Yeah, I talked to Anthony about that and he didn't like the idea of
>> devices knowing about their device tree specifics. Serial is a good
>> candidate to understand why. A serial device shouldn't know if it's
>> device 0 or device 1. It's just a plain device. The semantics on the
>> port go with the board.
> 
> Well, cell-index should probably just be dropped.  The board code (or at
> least code for the SoC family) should determine the alias name, but it
> shouldn't have to take care of everything.
> 
> Consider more complicated devices with various properties describing
> attributes of the device itself, that have to match what's actually
> implemented.  If there is anything that is board knowledge, provide a
> way for the board to add it as a supplement.

Right. We'll get there, just have to figure out a way to do this cleanly. This patch set is really just meant to move all the device tree compilation into QEMU, so we can then start meddling with it as we like without having to do both at once - the import of logic into QEMU and the refactoring of where to put that code.

For now, the least intrusive way is to not touch device models - and that's certainly a path I got blessing for from Anthony. For the next step, we'll just go and take it next :).


Alex

Patch

diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index 5ff2d24..493ad6e 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -81,6 +81,8 @@  static int mpc8544_load_device_tree(CPUPPCState *env,
     char compatible[] = "MPC8544DS\0MPC85xxDS";
     char model[] = "MPC8544DS";
     char soc[128];
+    char ser0[128];
+    char ser1[128];
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
     if (!filename) {
@@ -182,6 +184,7 @@  static int mpc8544_load_device_tree(CPUPPCState *env,
         }
     }
 
+    qemu_devtree_add_subnode(fdt, "/aliases");
     /* XXX These should go into their respective devices' code */
     sprintf(soc, "/soc8544@%x", MPC8544_CCSRBAR_BASE);
     qemu_devtree_add_subnode(fdt, soc);
@@ -196,6 +199,38 @@  static int mpc8544_load_device_tree(CPUPPCState *env,
     /* XXX should contain a reasonable value */
     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
 
+    /*
+     * We have to generate ser1 first, because Linux takes the first
+     * device it finds in the dt as serial output device. And we generate
+     * devices in reverse order to the dt.
+     */
+    sprintf(ser1, "%s/serial@%x", soc,
+            MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE);
+    qemu_devtree_add_subnode(fdt, ser1);
+    qemu_devtree_setprop_string(fdt, ser1, "device_type", "serial");
+    qemu_devtree_setprop_string(fdt, ser1, "compatible", "ns16550");
+    qemu_devtree_setprop_cell2(fdt, ser1, "reg", MPC8544_SERIAL1_REGS_BASE -
+                               MPC8544_CCSRBAR_BASE, 0x100);
+    qemu_devtree_setprop_cell(fdt, ser1, "cell-index", 1);
+    qemu_devtree_setprop_cell(fdt, ser1, "clock-frequency", 0);
+    qemu_devtree_setprop_cell2(fdt, ser1, "interrupts", 42, 2);
+    qemu_devtree_setprop_phandle(fdt, ser1, "interrupt-parent", mpic);
+    qemu_devtree_setprop_string(fdt, "/aliases", "serial1", ser1);
+
+    sprintf(ser0, "%s/serial@%x", soc,
+            MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE);
+    qemu_devtree_add_subnode(fdt, ser0);
+    qemu_devtree_setprop_string(fdt, ser0, "device_type", "serial");
+    qemu_devtree_setprop_string(fdt, ser0, "compatible", "ns16550");
+    qemu_devtree_setprop_cell2(fdt, ser0, "reg", MPC8544_SERIAL0_REGS_BASE -
+                               MPC8544_CCSRBAR_BASE, 0x100);
+    qemu_devtree_setprop_cell(fdt, ser0, "cell-index", 0);
+    qemu_devtree_setprop_cell(fdt, ser0, "clock-frequency", 0);
+    qemu_devtree_setprop_cell2(fdt, ser0, "interrupts", 42, 2);
+    qemu_devtree_setprop_phandle(fdt, ser0, "interrupt-parent", mpic);
+    qemu_devtree_setprop_string(fdt, "/aliases", "serial0", ser0);
+    qemu_devtree_setprop_string(fdt, "/chosen", "linux,stdout-path", ser0);
+
     ret = rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
     if (ret < 0) {
         goto out;
diff --git a/pc-bios/mpc8544ds.dts b/pc-bios/mpc8544ds.dts
index 01b53ba..e536ab1 100644
--- a/pc-bios/mpc8544ds.dts
+++ b/pc-bios/mpc8544ds.dts
@@ -12,32 +12,10 @@ 
 /dts-v1/;
 / {
 	aliases {
-		serial0 = &serial0;
-		serial1 = &serial1;
 		pci0 = &pci0;
 	};
 
 	soc8544@e0000000 {
-		serial0: serial@4500 {
-			cell-index = <0>;
-			device_type = "serial";
-			compatible = "ns16550";
-			reg = <0x4500 0x100>;
-			clock-frequency = <0>;
-			interrupts = <42 2>;
-			interrupt-parent = <&mpic>;
-		};
-
-		serial1: serial@4600 {
-			cell-index = <1>;
-			device_type = "serial";
-			compatible = "ns16550";
-			reg = <0x4600 0x100>;
-			clock-frequency = <0>;
-			interrupts = <42 2>;
-			interrupt-parent = <&mpic>;
-		};
-
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
@@ -85,8 +63,4 @@ 
 		#address-cells = <3>;
 		reg = <0xe0008000 0x1000>;
 	};
-
-	chosen {
-		linux,stdout-path = "/soc8544@e0000000/serial@4500";
-	};
 };