Subject: Re: [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

Daniel Drake wrote:
> Hi,
Hi,

> Context: https://patchwork.kernel.org/patch/450681/
>
> This patch will indeed cause problems for OLPC. Thanks for bringing it
> to our attention.
>
> On OLPC, the device tree is not used as a source of devices like on
> other platforms, it is simply used to present information to the
> kernel and userspace (in read-only fashion).
>
> If I understand it correctly, the above patch is saying: if we have a
> device tree, don't add the standard x86 RTC device.
Yes.

> However, what we need it to say is: if we have a device tree *and* the
> device tree is being used as a source of devices, don't add the
> standard x86 RTC device.
>
> Therefore in the OLPC case, this particular bail-out condition will
> never be met, because the device tree is not being used as a source of
> devices.
So it is not case now. Will it ever be?

>
> Does that make sense?

I don't quite get how or what for do you use the device tree. Could you
please answer me the following questions:
- is the variable allnodes NULL in your case?
- variable initial_boot_params should be NULL in your case, right?
- how should I checked for "device tree is being used as a source of
devices"? The nodes on in the device tree are not probed unless one
calls of_platform_bus_probe() with a few ids. However I do this now
unconditionally which is not a problem unless you have a device tree ...

> Thanks,
> Daniel

Sebastian


2011-02-01 18:36:34

by Andres Salomon

[permalink] [raw]
Subject: Re: [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On Tue, 01 Feb 2011 14:21:22 +0100
Sebastian Andrzej Siewior <[email protected]> wrote:

> Daniel Drake wrote:
> > Hi,
> Hi,
>
> > Context: https://patchwork.kernel.org/patch/450681/
> >
> > This patch will indeed cause problems for OLPC. Thanks for bringing
> > it to our attention.
> >
> > On OLPC, the device tree is not used as a source of devices like on
> > other platforms, it is simply used to present information to the
> > kernel and userspace (in read-only fashion).
> >
> > If I understand it correctly, the above patch is saying: if we have
> > a device tree, don't add the standard x86 RTC device.
> Yes.
>
> > However, what we need it to say is: if we have a device tree *and*
> > the device tree is being used as a source of devices, don't add the
> > standard x86 RTC device.
> >
> > Therefore in the OLPC case, this particular bail-out condition will
> > never be met, because the device tree is not being used as a source
> > of devices.
> So it is not case now. Will it ever be?
>

That is unclear. For now, it's not, and there aren't plans to make it
so.

> >
> > Does that make sense?
>
> I don't quite get how or what for do you use the device tree. Could
> you please answer me the following questions:
> - is the variable allnodes NULL in your case?

No.

> - variable initial_boot_params should be NULL in your case, right?

Yes.

> - how should I checked for "device tree is being used as a source of
> devices"? The nodes on in the device tree are not probed unless one
> calls of_platform_bus_probe() with a few ids. However I do this now
> unconditionally which is not a problem unless you have a device
> tree ...

Perhaps it should be specifically checking for a fdt (by way of
initial_boot_params)? Sparc also does not have initial_boot_params,
so one might even be able to drop an #ifdef in the process.

2011-02-02 03:10:36

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On Tue, Feb 1, 2011 at 11:36 AM, Andres Salomon <[email protected]> wrote:
> On Tue, 01 Feb 2011 14:21:22 +0100
> Sebastian Andrzej Siewior <[email protected]> wrote:
>
>> Daniel Drake wrote:
>> > Hi,
>> Hi,
>>
>> > Context: https://patchwork.kernel.org/patch/450681/
>> >
>> > This patch will indeed cause problems for OLPC. Thanks for bringing
>> > it to our attention.
>> >
>> > On OLPC, the device tree is not used as a source of devices like on
>> > other platforms, it is simply used to present information to the
>> > kernel and userspace (in read-only fashion).
>> >
>> > If I understand it correctly, the above patch is saying: if we have
>> > a device tree, don't add the standard x86 RTC device.
>> Yes.
>>
>> > However, what we need it to say is: if we have a device tree *and*
>> > the device tree is being used as a source of devices, don't add the
>> > standard x86 RTC device.
>> >
>> > Therefore in the OLPC case, this particular bail-out condition will
>> > never be met, because the device tree is not being used as a source
>> > of devices.
>> So it is not case now. Will it ever be?
>>
>
> That is unclear. ?For now, it's not, and there aren't plans to make it
> so.
>
>> >
>> > Does that make sense?
>>
>> I don't quite get how or what for do you use the device tree. Could
>> you please answer me the following questions:
>> - is the variable allnodes NULL in your case?
>
> No.
>
>> - variable initial_boot_params should be NULL in your case, right?
>
> Yes.
>
>> - how should I checked for "device tree is being used as a source of
>> ? ?devices"? The nodes on in the device tree are not probed unless one
>> ? ?calls of_platform_bus_probe() with a few ids. However I do this now
>> ? ?unconditionally which is not a problem unless you have a device
>> tree ...
>
> Perhaps it should be specifically checking for a fdt (by way of
> initial_boot_params)? ? Sparc also does not have initial_boot_params,
> so one might even be able to drop an #ifdef in the process.

OLPC is very much the oddball in this case. Everyone else uses
devicetree for registering devices. It could be solved by making OLPC
explicitly register the RTC.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Subject: Re: [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

Grant Likely wrote:
>> Perhaps it should be specifically checking for a fdt (by way of
>> initial_boot_params)? Sparc also does not have initial_boot_params,
>> so one might even be able to drop an #ifdef in the process.
>
> OLPC is very much the oddball in this case. Everyone else uses
> devicetree for registering devices. It could be solved by making OLPC
> explicitly register the RTC.

I second that :)

>
> g.
>

Sebastian

2011-02-03 18:40:57

by Andres Salomon

[permalink] [raw]
Subject: Re: [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On Tue, 1 Feb 2011 20:10:13 -0700
Grant Likely <[email protected]> wrote:

> On Tue, Feb 1, 2011 at 11:36 AM, Andres Salomon <[email protected]>
> wrote:
> > On Tue, 01 Feb 2011 14:21:22 +0100
> > Sebastian Andrzej Siewior <[email protected]> wrote:
> >
> >> Daniel Drake wrote:
> >> > Hi,
> >> Hi,
> >>
> >> > Context: https://patchwork.kernel.org/patch/450681/
> >> >
> >> > This patch will indeed cause problems for OLPC. Thanks for
> >> > bringing it to our attention.
> >> >
> >> > On OLPC, the device tree is not used as a source of devices like
> >> > on other platforms, it is simply used to present information to
> >> > the kernel and userspace (in read-only fashion).
> >> >
> >> > If I understand it correctly, the above patch is saying: if we
> >> > have a device tree, don't add the standard x86 RTC device.
> >> Yes.
> >>
> >> > However, what we need it to say is: if we have a device tree
> >> > *and* the device tree is being used as a source of devices,
> >> > don't add the standard x86 RTC device.
> >> >
> >> > Therefore in the OLPC case, this particular bail-out condition
> >> > will never be met, because the device tree is not being used as
> >> > a source of devices.
> >> So it is not case now. Will it ever be?
> >>
> >
> > That is unclear.  For now, it's not, and there aren't plans to make
> > it so.
> >
> >> >
> >> > Does that make sense?
> >>
> >> I don't quite get how or what for do you use the device tree. Could
> >> you please answer me the following questions:
> >> - is the variable allnodes NULL in your case?
> >
> > No.
> >
> >> - variable initial_boot_params should be NULL in your case, right?
> >
> > Yes.
> >
> >> - how should I checked for "device tree is being used as a source
> >> of devices"? The nodes on in the device tree are not probed unless
> >> one calls of_platform_bus_probe() with a few ids. However I do
> >> this now unconditionally which is not a problem unless you have a
> >> device tree ...
> >
> > Perhaps it should be specifically checking for a fdt (by way of
> > initial_boot_params)?   Sparc also does not have
> > initial_boot_params, so one might even be able to drop an #ifdef in
> > the process.
>
> OLPC is very much the oddball in this case. Everyone else uses
> devicetree for registering devices. It could be solved by making OLPC
> explicitly register the RTC.
>
> g.
>

That's actually what the plan is; the code just depends on PM features,
which is still being worked on.

2011-02-03 19:44:16

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [sodaville] [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On 02/03/2011 10:40 AM, Andres Salomon wrote:
>
> That's actually what the plan is; the code just depends on PM features,
> which is still being worked on.
>

I really don't understand what this is supposed to mean in this context
at all.

-hpa

2011-02-03 20:09:47

by Andres Salomon

[permalink] [raw]
Subject: Re: [sodaville] [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On Thu, 03 Feb 2011 11:44:15 -0800
"H. Peter Anvin" <[email protected]> wrote:

> On 02/03/2011 10:40 AM, Andres Salomon wrote:
> >
> > That's actually what the plan is; the code just depends on PM
> > features, which is still being worked on.
> >
>
> I really don't understand what this is supposed to mean in this
> context at all.
>
> -hpa


Daniel has a patch set that includes the following:

0010-OLPC-add-XO-1-rtc-driver.patch

+++ b/arch/x86/platform/olpc/olpc.c
@@ -311,6 +311,8 @@ static int __init add_xo1_platform_devices(void)
{
struct platform_device *pdev;

+ olpc_xo1_rtc_init();
+
pdev = platform_device_register_simple("xo1-rfkill", -1, NULL,
0);


This registers a specific rtc device for XO-1 machines (using the
CS5536 RTC). XO-1.5 uses ACPI and a Via-based board, so the RTC device
should be handled there automatically. The corner case is for machines
where the OLPC DT is enabled, but OLPC_XO1_RTC is not enabled. We
still need to figure out what to do in that case, but I think it's more
appropriate to focus on getting the PM and XO1_RTC stuff upstream
first. As of right now, it's the *only* case. We'll change that, by
getting OLPC_XO1_RTC upstream.

2011-02-03 20:16:39

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [sodaville] [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On 02/03/2011 12:09 PM, Andres Salomon wrote:
>
> Daniel has a patch set that includes the following:
>
> 0010-OLPC-add-XO-1-rtc-driver.patch
>
> +++ b/arch/x86/platform/olpc/olpc.c
> @@ -311,6 +311,8 @@ static int __init add_xo1_platform_devices(void)
> {
> struct platform_device *pdev;
>
> + olpc_xo1_rtc_init();
> +
> pdev = platform_device_register_simple("xo1-rfkill", -1, NULL,
> 0);
>
>
> This registers a specific rtc device for XO-1 machines (using the
> CS5536 RTC). XO-1.5 uses ACPI and a Via-based board, so the RTC device
> should be handled there automatically. The corner case is for machines
> where the OLPC DT is enabled, but OLPC_XO1_RTC is not enabled. We
> still need to figure out what to do in that case, but I think it's more
> appropriate to focus on getting the PM and XO1_RTC stuff upstream
> first. As of right now, it's the *only* case. We'll change that, by
> getting OLPC_XO1_RTC upstream.

OK, so how about "you're -----d anyway, so please don't block other
development because you haven't gotten your own driver upstream?"

-hpa

2011-02-03 20:39:52

by Andres Salomon

[permalink] [raw]
Subject: Re: [sodaville] [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On Thu, 03 Feb 2011 12:16:24 -0800
"H. Peter Anvin" <[email protected]> wrote:

> On 02/03/2011 12:09 PM, Andres Salomon wrote:
> >
> > Daniel has a patch set that includes the following:
> >
> > 0010-OLPC-add-XO-1-rtc-driver.patch
> >
> > +++ b/arch/x86/platform/olpc/olpc.c
> > @@ -311,6 +311,8 @@ static int __init add_xo1_platform_devices(void)
> > {
> > struct platform_device *pdev;
> >
> > + olpc_xo1_rtc_init();
> > +
> > pdev = platform_device_register_simple("xo1-rfkill", -1,
> > NULL, 0);
> >
> >
> > This registers a specific rtc device for XO-1 machines (using the
> > CS5536 RTC). XO-1.5 uses ACPI and a Via-based board, so the RTC
> > device should be handled there automatically. The corner case is
> > for machines where the OLPC DT is enabled, but OLPC_XO1_RTC is not
> > enabled. We still need to figure out what to do in that case, but
> > I think it's more appropriate to focus on getting the PM and
> > XO1_RTC stuff upstream first. As of right now, it's the *only*
> > case. We'll change that, by getting OLPC_XO1_RTC upstream.
>
> OK, so how about "you're -----d anyway, so please don't block other
> development because you haven't gotten your own driver upstream?"
>
> -hpa

Sorry if I hadn't made that clear, but that's what I was saying. Go
ahead with the x86 DT stuff, we'll deal with OLPC breakage later.

2011-02-03 21:04:25

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [sodaville] [PATCH v2 01/15] x86/e820: remove conditional early mapping in parse_e820_ext

On 02/03/2011 12:39 PM, Andres Salomon wrote:
> Sorry if I hadn't made that clear, but that's what I was saying. Go
> ahead with the x86 DT stuff, we'll deal with OLPC breakage later.

Ah, ok, cool.

-hpa