lkml.org 
[lkml]   [2010]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [sodaville] [PATCH 03/11] x86/dtb: Add a device tree for CE4100
Benjamin Herrenschmidt wrote:

>>> Also how do you plan to expose threading capability ?
>> I haven't plan because this CPU has to threading capability. If there
>> is, I would follow the powerpc way (unless it is allready documented how
>> to do so on x86).
>
> Atoms have SMT don't they ?

It is available on some Atom CPUs. This one does not support it. It has
just one CPU with one thread.

>>> You probably also want some linkage from the processor to the local APIC
>>> no ?
>> Like now I walk through the device tree and look for one but that sounds
>> like a good idea.
>
> The day you have multiple Atom's on a board the "looking for one" won't
> work well :-) Better have explicit references whenever you can for that
> type of linkage.

This also works with the flat tree, right?

>>> Also APICs have some kind od
>>> versionning, they aren't all identical, so your compatible property
>>> needs to be more precise at least.
>> The APIC has a register where you can read the version of the chip, yes.
>> You want me to add this version into the compatible field?
>
> Ideally, you should add something like
>
> intel,ioapic-atomXXX intel-ioapic-vYY intel-ioapic
>
> IE. From the most specific to the most generic. That way if a "quirk" is
> ever needed due to an errata specific to that chip model that isn't
> directly covered by the "version", you get to use that too (unless that
> version register also contains things like mask number etc... in which
> case it's probably enough).

Okay, so we want this for a quirk at a later point in time. Now I
understand.

>>>> + isa@legacy {
>>> So ISA isn't a child of "atom"... that makes "atom" a bit strange as a
>>> node, tho not a big deal per se. I suppose it represent the on-die
>>> peripherals but then you need at least some linkage between that and
>>> the /cpus nodes.
>> Yes, it should represent the on-die peripherals. How should that look
>> like? The bus the same level as the cpu node and link from the cpu to
>> the isa bus?
>
> I would make isa a child of atom
Okay.

>>>> + device_type = "isa";
>>>> + compatible = "simple-bus";
>>> What does "simple-bus" means ?
>> I added simple bus in order to get probed. But I now I rember that this
>> is also supported per device_type. I get rid of it.
>
> device_type is a nasty bugger, we are trying to get rid of Linux
> reliance on it.
>
> Things like "simple-bus" don't rock my boat either, it's adding to the
> device-tree "informations" that are specific to the way Linux will
> interpret it, which is not how it should be.
>
> In this case I would have said something like "atom,isa-bridge" but
> heh...

Would "isa-bridge" be acceptable? So I don't have to add a new bus to the
probe list for every new SoC.

>>>> + rtc@legacy {
>>>> + compatible = "motorola,mc146818";
>>>> + interrupts = <8 3>;
>>>> + interrupt-parent = <&ioapic1>;
>>>> + ctrl_reg = <2>;
>>>> + freq_reg = <0x26>;
>>>> + reg = <1 0x70 2>;
>>>> + };

>>> Also, "ctrl_reg" and "freq_reg" follow an existing binding ? If not,
>>> then I'd suggest you use "-" instead of "_" which is more common in OFW
>>> land and use more descriptive names since "reg" has a meaning of its own
>>> and the above is a bit confusing.

> I definitely agree that it's much better to use a property in the
> device-tree and I'n not arguing against it :-) I was merely asking
> whether that property name was already defined somewhere and if not,
> suggesting a different naming (using dashes rather than underscores)
> which is more consistent with traditional usage :-)

Okay, so I replace _ with - in ctrl_reg and freq_reg if this is your only
concern.

> (Oh and maybe publish a binding wherever Grant puts these nowadays while
> at it so we can all do the same thing from now on)
Good.

>>>> + pci@3fc {
>>>> + /* Secondary IO-APIC */
>>>> + ioapic2: pic@bffff000 {
>>>> + compatible = "intel,ioapic";
>>>> + reg = <0x100 0x0 0x0 0x0>;
>>>> + phys_reg = <0xbffff000>;
>>>> + };

>> The reg property contains the devfn number, interrupt mask, pin number.
>
> No. The "reg" property contains among other things the devfn, but
> certainly not the interrupt mask and pin number. I recommend you look at
> the PCI binding for OF, it's actually not very complicated :-)
>
> The "reg" basically contains an entry for the "config space" of the
> device which basically represents the devfn only, and an entry for each
> BAR which contains the size and various attributes (not the assigned
> address tho, this goes into a separate assigned-addresses property).
>
>> That is what I've been seeing in PCI nodes. phys_reg is the physical
>> address of the chip since reg is allready taken and PCI is not yet up
>> (as I allready explained).
>
> Right but with the appropriate assigned-addresses property, you can
> represent that using standard properties (and use existing address
> resolution helpers from drivers/of) without inventing a new "phys_reg"
> which btw has issues too ("reg" traditionally is a tupple addr/size,
> also where is the number of cells used in phys_reg defined ?).
phys_reg does not have it. And probably won't since we eliminated it.

>>>> + i2c@15a00 {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + reg = <0x15a00 0x0 0x0 0x0>;
>>> OFW PCI binding, which we follow, mandates an "assigned-addresses"
>>> property, tho I suppose that if you haven't assigned anything yet (and
>>> will let Linux do so) the above is kosher. Your "reg" is a bit odd but I
>>> don't have time to dbl check vs. the binding right now.
>> reg is devfn. I just looked up "assigned-addresses" in the PCI BUS Spec
>> and it looks like what I could use instead of phys-reg property. So if
>> this is the case then I need to to distinguish between the first on
>> secondary ioapic and go either for the reg property or
>> assigned-addresses.
>
> You don't really need to. There's code that will do that for you :-)
>
> Stuff in drivers/of/address.c shall be able to parse the addresses by
> index (tho I suppose you might still want to do a special case for PCI
> since the natural way to get to a PCI based address is via a BAR number
> while other stuff just takes an address index).
>
> You shouldn't ever have to look directly at "reg" or
> "assigned-addresses" yourself.

Yes. of_address_to_resource() will do the right thing in this case. It can
only be used after unflatten_device_tree() and I need this earlier.
Now using unflatten_device_tree() earlier isn't that easy, or is it.
I defered the ioapic init a little, so it is now called from
x86_init.mpparse.get_smp_config() so I have alloc_bootmem() working.
So unflatten_device_tree() seems to work here. The ugly part comes now:
early_init_dt_alloc_memory_arch() expects u64 which works with
phys_to_virt() and the other way around. This isn't really the case with
what __alloc_bootmem(). This looks like phys_map to me. Since the dtb code
simply uses phys_to_virt() it doesn't really matter. So it works and I
probably can use of_address_to_resource().

> Cheers,
> Ben.

Sebastian


\
 
 \ /
  Last update: 2010-11-29 20:39    [W:0.080 / U:1.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site