Messages in this thread | | | Date | Mon, 29 Nov 2010 20:36:17 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [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
| |