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

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

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

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

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

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