Re: [sodaville] [PATCH 03/11] x86/dtb: Add a device tree for CE4100

[Sebastian Andrzej Siewior]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/