Re: [PATCH v4 3/4] of: Documentation: Add x86 local APIC ID property

[Rob Herring]

On Wed, Mar 7, 2018 at 7:11 PM, Ivan Gorinov <ivan.gorinov@xxxxxxxxx> wrote:
> On Wed, 2018-03-07 at 14:23 -0600, Rob Herring wrote:
>
>> > Add new "intel,apic-id" property to allow using CPU descriptions
>> > in Device Tree data provided by the U-Boot loader.
>> > Address specified in 'reg' to be used as default local APIC ID
>> > to avoid breaking existing systems with DTB provided by firmware.
>> Is there some reason to not always use reg? For when the numbering of
>> cpus and timers is different?
>
> Yes, local APIC ID may differ from CPU number.
> For example, in Atom E38xx (u-boot/arch/x86/dts/minnowmax.dts):
>
>         cpus {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 cpu@0 {
>                         device_type = "cpu";
>                         compatible = "intel,baytrail-cpu";
>                         reg = <0>;
>                         intel,apic-id = <0>;
>                 };
>
>                 cpu@1 {
>                         device_type = "cpu";
>                         compatible = "intel,baytrail-cpu";
>                         reg = <1>;
>                         intel,apic-id = <4>;
>                 };
>         };
>
>> Of course, we do have the situation on ARM with the GIC that the GIC
>> CPU IDs may be
>> >
>> >
>> > Signed-off-by: Ivan Gorinov <ivan.gorinov@xxxxxxxxx>
>> > ---
>> >  Documentation/devicetree/bindings/x86/ce4100.txt | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
>> > index b49ae59..d15de48 100644
>> > --- a/Documentation/devicetree/bindings/x86/ce4100.txt
>> > +++ b/Documentation/devicetree/bindings/x86/ce4100.txt
>> > @@ -14,11 +14,17 @@ The CPU node
>> >                 compatible = "intel,ce4100";
>> >                 reg = <0>;
>> >                 lapic = <&lapic0>;
>> Isn't this enough? I can't tell because whatever this points to has no
>> binding documentation.
>
> Local APIC is a part of CPU, not an external device (except for 486 and early Pentium).
> Every CPU has access to its own local APIC registers at the same base address (0xfee00000).
> Therefore, one "lapic" device node can work for all processors in the system.

Do you need a lapic node then? If you typically don't have a node,
then just having the id should be fine.

> With more changes in the code, the local APIC description could be made optional
> because every processor can always read its local APIC base address from MSR 0x1b.
> And when x2APIC mode is enabled, the local APIC registers are accessed as model
> specific registers instead of memory-mapped I/O.
>
>> You could perhaps extend it and add a cell with the id value.
>
> This may require different DT data for Linux and U-Boot, or changes in the latter.

The latter case. There's one upstream for DT binding reviews.

Rob