Oops: of_platform_serial_probe

[Arnd Bergmann]

On Tuesday 20 November 2007, Clemens Koller wrote:
> The device tree is the default one which comes with the kernel:
> paulus.git/arch/powerpc/boot/dts/mpc8540ads.dts
> which contains:
> 
>                  serial at 4500 {
>                          device_type = "serial";
>                          compatible = "ns16550";
>                          reg = <4500 100>;       // reg base, size
>                          clock-frequency = <0>;  // should we fill in in uboot?
>                          interrupts = <2a 2>;
>                          interrupt-parent = <&mpic>;
>                  };
> 
>                  serial at 4600 {
>                          device_type = "serial";
>                          compatible = "ns16550";
>                          reg = <4600 100>;       // reg base, size
>                          clock-frequency = <0>;  // should we fill in in uboot?
>                          interrupts = <2a 2>;
>                          interrupt-parent = <&mpic>;
>                  };
> 
>  > One potential problem that I can see is a missing 'current-speed'
>  > property in your tree, which would cause this behavior.
> 
> That's correct. Should be fixed in all .dts' ?
 
It depends a lot on how the systems are used. current-speed only makes
sense if there is a boot loader that also does some serial I/O at a
speed it sets up itself.


>  > It looks
>  > like many device trees set this, but it is not required by all
>  > bindings.
> 
> How should someone know, when it's really needed and when not?

The point of current-speed is that the kernel can tell what bitrate
was used by the boot loader and set up the same speed so that your
terminal emulation does not get garbled output when changing from
boot loader messages to kernel messages.

>  > If that's the case, the patch below should fix your
>  > problem, but you probably want to set the current-speed anyway,
>  > according to your boot loader settings.
> 
> I think there was no need to set it again, because of: console=ttyS0,115200
> But I'll verify...

When current-speed is set, you don't need to override the speed at the
command line.

> @@ -36,6 +36,10 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>         memset(port, 0, sizeof *port);
>         spd = of_get_property(np, "current-speed", NULL);
>         clk = of_get_property(np, "clock-frequency", NULL);
> +       if (!spd) {
> +               dev_warn(&ofdev->dev, "no current-speed property set\n");
> +               return -ENODEV;
> +       }
>         if (!clk) {
>                 dev_warn(&ofdev->dev, "no clock-frequency property set\n");
>                 return -ENODEV;

This looks wrong. Since the current-speed property is not mandated by open firmware,
we should not error out here, but simply use the setting from the command line
or whatever other defaults can be used. Not setting port->custom_divisor at all
should do the job.

	Arnd <><