linux-kernel - Re: [sodaville] [PATCH TIP v2 03/14] x86/dtb: Add a device tree for CE4100

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20110222112112.GA24641@www.tglx.de>
Date:	Tue, 22 Feb 2011 12:21:12 +0100
From:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	sodaville@...utronix.de, devicetree-discuss@...ts.ozlabs.org,
	x86@...nel.org, linux-kernel@...r.kernel.org,
	David Gibson <david@...son.dropbear.id.au>
Subject: Re: [sodaville] [PATCH TIP v2 03/14] x86/dtb: Add a device tree for CE4100

* Grant Likely | 2011-02-16 14:44:28 [-0700]:

>> diff --git a/arch/x86/platform/ce4100/falconfalls.dts b/arch/x86/platform/ce4100/falconfalls.dts
>> new file mode 100644
>> index 0000000..e888657
>> --- /dev/null
>> +++ b/arch/x86/platform/ce4100/falconfalls.dts
>> @@ -0,0 +1,424 @@
>> +	soc@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "intel,ce4100-cp";
>
>You *must* include documentation for each of these new compatible
>values before merging this patch.  At the very least it acts as a
>placeholder and describes exactly what the string describes.

I added something.

>> +		ranges;
>> +
>> +		ioapic1: interrupt-controller@...00000 {
>> +			#interrupt-cells = <2>;
>> +			compatible = "intel,ioapic-ce4100", "intel,ioapic";
>
>"intel,ioapic" is probably too generic and can be dropped.  Newer
>devices can claim compatibility with "intel,ioapic-ce4100" if they are
>indeed compatible so that device drivers don't need to be modified.
>It is better to anchor compatible values to real implementations that
>try to come up with 'generic' or wildcard strings.  Ditto through the
>rest of the file.
>
>"intel,ce4100-ioapic" (instead of intel,ioapic-ce4100) would follow
>current convention better of specifying the chip first, followed by
>the on-chip device.
>
Done.

>> +			pci@1,0 {
>> +				#address-cells = <3>;
>> +				#interrupt-cells = <1>;
>> +				#size-cells = <2>;
>> +				compatible = "intel,ce4100-pci", "pci";
>> +				device_type = "pci";
>> +				bus-range = <1 1>;
>> +				ranges = <0x2000000 0 0xdffe0000 0x2000000 0 0xdffe0000 0 0x1000>;
>> +
>> +				interrupt-parent = <&ioapic2>;
>> +
>> +				display@2,0 {
>> +					compatible = "pci8086,2e5b.2",
>> +						   "pci8086,2e5b",
>> +						   "pciclass038000",
>> +						   "pciclass0380";
>> +
>> +					reg = <0x11000 0x0 0x0 0x0 0x0>;
>> +					interrupts = <0 1>;
>> +				};
>
>Here's where things get a little sticky for PCI child devices
>(probably should have thought about this earlier, sorry).  Are these
>devices runtime detectable?  ie. does reading the PCI BARs and irq
>configuration registers reflect reality?

Bar yes (no assigned-property here), IRQ no.

This devices are runtime detectable via the PCI bus. However the
interrupt number is no longer correct. The PCI bus reports the legacy
number which is irq 4 (if I remember correctly) for all devices here.
Which is correct for the legacy mode. Those interrupts are routed to the
legacy interrupt controller. Once we activate the IO APIC for interrupt
routing the legacy controller is no longer in use. With the IO APIC
there is a different interrupt routing which is described here by the
interrupt property.
On systems with ACPI this kind of information is read fron there. You
see during boot-up "PCI->APIC IRQ transform: ...".

>In general, if something can be *reliably* detected then it should not
>be listed in the device tree.  However, if there is some horrid gotcha
>that prevents it being detected reliably, then it definitely should
>appear here.

The first edition used the interrupt-map property to describe this
mapping. David Gibson [0] recommended then to create device nodes
because the PCI INTA..D lines were not used.

>I cannot answer this question for you since I don't know the hardware.
>It's a judgement call that you'll need to make on whether or not these
>PCI (or pseudo pci?) devices should be listed in the dt.

Most of the devices can be detected reliably. I2C and SPI need some
additional information. I believe more of this things will pop up once
someone adds the GPIO wires which are not there yet. I know that the
smartcard thingy uses one GPIO for card detect. So I would need to
extend the device property later for that :)

>One of the things that does need to be improved is partial probing of
>PCI busses so that only the messy PCI devices get listed in the device
>tree, and the rest of the devices can get detected in the normal
>manner.

I did not add any "special" compatible properties just the pci ids. So
they should not be probed via DT just PCI unless one adds this to the
driver.

[0] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004137.html

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/