Re: [RFC PATCH arm: initial TI-Nspire support]

[Arnd Bergmann <arnd@arndb.de>]

On Sunday 07 April 2013, Daniel Tang wrote:
> On 07/04/2013, at 12:24 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> >> @@ -313,7 +314,7 @@ define archhelp
> >>   echo  '  Image         - Uncompressed kernel image (arch/$(ARCH)/boot/Image)'
> >>   echo  '* xipImage      - XIP kernel image, if configured (arch/$(ARCH)/boot/xipImage)'
> >>   echo  '  uImage        - U-Boot wrapped zImage'
> >> -  echo  '  bootpImage    - Combined zImage and initial RAM disk' 
> >> +  echo  '  bootpImage    - Combined zImage and initial RAM disk'
> >>   echo  '                  (supply initrd image via make variable INITRD=<path>)'
> >>   echo  '* dtbs          - Build device tree blobs for enabled boards'
> >>   echo  '  install       - Install uncompressed kernel'
> > 
> > This looks like it wasn't meant to be in the patch.
> 
> It probably isn't. I think there was trailing whitespace on that and my editor happened to remove it automatically.
> 
> Should this be a separate patch to fix up formatting or should I leave it in as a drive-by fix?

Any cleanups like this should be separate patches, and ideally even
part of a different patch series.

> > 
> >> +			uart: uart@90020000 {
> >> +				reg = <0x90020000 0x1000>;
> >> +				interrupts = <1>;
> >> +
> >> +				clocks = <&uart_clk>;
> >> +				clock-names = "uart_clk";
> >> +			};
> > 
> > The name for a uart should be "serial". Since this is a pl01x, please add
> > the required properties for the device, e.g. 
> > 
> > 	compatible = "arm,pl011", "arm,primecell";
> > 
> > You will need the "arm,primecell" bit to make the device appear on the
> > amba bus rather than the platform bus.
> 
> That was actually deliberate because different models of the TI-NSPIRE have different
> serial hardware. On the newer CX models, it is a PL01x and on the older models, it has
> a 8250-like interface. They all reside at the same address with the same IRQ though.
> 
> I thought it might be cleaner to specify the interrupts and registers in the common file
> and leave it to the board specific ones to implement the "compatible" property.

I see. It seems a little confusing to the reader though. I don't have a good answer,
but there are two other options how this can be handled:

* Put both devices in the devicetree and mark them as status="disabled" in the main file,
but enable one of them in the version specific files.

* leave them out of the .dtsi file and only define them in the specific .dts files.
> >> +	err = of_property_read_string(of_aliases, "timer0", &path);
> >> +	if (WARN_ON(err))
> >> +		return;
> >> +
> >> +	timer = of_find_node_by_path(path);
> >> +	base = of_iomap(timer, 0);
> >> +	if (WARN_ON(!base))
> >> +		return;
> >> +
> >> +	clk = of_clk_get_by_name(timer, NULL);
> >> +	clk_register_clkdev(clk, timer->name, "sp804");
> >> +
> >> +	sp804_clocksource_init(base, timer->name);
> >> +
> >> +	err = of_property_read_string(of_aliases, "timer1", &path);
> >> +	if (WARN_ON(err))
> >> +		return;
> > 
> > In particular, I think the method of using aliases to pick the right sp804
> > instance is being deprecated now. If both timers are identical, the kernel
> > will now just pick one of them.
> 
> Sorry, I don't quite understand. 
> 
> Out of the timers, I want to add one as a clocksource and one as a clockevent.
> If they're identical (i.e. without using aliases), how should I tell the kernel,
> "Take the first timer you see and make it a clocksource, take the next one you
> see and make it a clockevent"?

The modified sp804 driver will have logic to do that. I think in the end we
decided that the driver should first look for any device that can be used as
a clocksource and use it that way. If it finds a second device, that can be
used as clockevent.

	Arnd