On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov <[email protected]> wrote:
> On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
>> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>> > When called with a non-zero of_node, fill out a new ramoops_platform_data
>> > with data from the specified Flattened Device Tree node.
>> > Update ramoops documentation with the new FDT interface.
>> > Update devicetree/binding documentation with pstore/ramoops.txt.
>>
>> Thanks for your work, Bryan! There were a few issues, I fixed
>> them myself but I need your confirmation if you're OK w/ all
>> the changes.
> [...]
>> So, the resulting patch is down below. But I have not pushed
>> it out yet, I'm waiting for your sign off. If you're OK with the
>> changes, please reply with
>> 'Signed-off-by: Bryan Freed <[email protected]>'
>
> Bryan, have you had a chance to look into this?
Whatever happened to this? Olof was also looking at doing a binding
back in Jan 2012:
http://www.mail-archive.com/[email protected]/msg09905.html
Rob
On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov <[email protected]> wrote:
> > On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
> >> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
> >> > When called with a non-zero of_node, fill out a new ramoops_platform_data
> >> > with data from the specified Flattened Device Tree node.
> >> > Update ramoops documentation with the new FDT interface.
> >> > Update devicetree/binding documentation with pstore/ramoops.txt.
> >>
> >> Thanks for your work, Bryan! There were a few issues, I fixed
> >> them myself but I need your confirmation if you're OK w/ all
> >> the changes.
> > [...]
> >> So, the resulting patch is down below. But I have not pushed
> >> it out yet, I'm waiting for your sign off. If you're OK with the
> >> changes, please reply with
> >> 'Signed-off-by: Bryan Freed <[email protected]>'
> >
> > Bryan, have you had a chance to look into this?
>
> Whatever happened to this? Olof was also looking at doing a binding
> back in Jan 2012:
>
> http://www.mail-archive.com/[email protected]/msg09905.html
I don't know. Bryan's patch looked good, I'd happily apply it. But I still
need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
behalf (anyone from Chromium project?). Or redo the patch.
Thanks,
Anton
Sorry for dropping the ball on this one, Anton.
Thank you for your feedback and modifications in the code.
I gotta ask, however, why do you completely remove key ramoops fields
like record_size and ftrace_size?
>From your 9/7/2012 comments (again, sorry for the delay in getting back to you):
> Personally, I don't see how this fits into device tree. It doesn't
> describe hardware, instead it's more a configuration stuff, which
> usually we try to not put into the device tree.
>
> It would be better to have a sane defaults in ramoops, instead of
> introducing more "virtual" stuff in the device tree. That is, feel
> free to change defaults if they seem to be not enough for most your
> setups.
So there are only two alternatives I can see to set these fields.
1. Pass kernel command line parameters, or
2. Modify the defaults in ram.c.
Neither of these alternatives is particularly clean to me. The first
is shunned to avoid clutter on the command line.
And the second requires us to maintain our own version of ram.c or
push our defaults on the rest of the community.
We (ok, maybe just 'I') prefer to keep our device specific values in
the device tree, even though it is not strictly defining hardware in
the system.
What do you think of at least keeping the record_size, console_size
and ftrace_size fields as optional in the ramoops device tree
specification?
And as a more general question, why should we try not to put
configuration in the device tree? It seems like a great (and
portable) place to put this stuff.
It certainly seems better to have it there than hardwired in the
kernel or tacked onto the kernel command line.
Thanks,
bryan.
On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov <[email protected]> wrote:
> On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
>> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov <[email protected]> wrote:
>> > On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
>> >> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>> >> > When called with a non-zero of_node, fill out a new ramoops_platform_data
>> >> > with data from the specified Flattened Device Tree node.
>> >> > Update ramoops documentation with the new FDT interface.
>> >> > Update devicetree/binding documentation with pstore/ramoops.txt.
>> >>
>> >> Thanks for your work, Bryan! There were a few issues, I fixed
>> >> them myself but I need your confirmation if you're OK w/ all
>> >> the changes.
>> > [...]
>> >> So, the resulting patch is down below. But I have not pushed
>> >> it out yet, I'm waiting for your sign off. If you're OK with the
>> >> changes, please reply with
>> >> 'Signed-off-by: Bryan Freed <[email protected]>'
>> >
>> > Bryan, have you had a chance to look into this?
>>
>> Whatever happened to this? Olof was also looking at doing a binding
>> back in Jan 2012:
>>
>> http://www.mail-archive.com/[email protected]/msg09905.html
>
> I don't know. Bryan's patch looked good, I'd happily apply it. But I still
> need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
> behalf (anyone from Chromium project?). Or redo the patch.
>
> Thanks,
>
> Anton
On 04/08/2013 02:54 PM, Bryan Freed wrote:
> Sorry for dropping the ball on this one, Anton.
>
> Thank you for your feedback and modifications in the code.
> I gotta ask, however, why do you completely remove key ramoops fields
> like record_size and ftrace_size?
>
> From your 9/7/2012 comments (again, sorry for the delay in getting back to you):
>> Personally, I don't see how this fits into device tree. It doesn't
>> describe hardware, instead it's more a configuration stuff, which
>> usually we try to not put into the device tree.
>>
>> It would be better to have a sane defaults in ramoops, instead of
>> introducing more "virtual" stuff in the device tree. That is, feel
>> free to change defaults if they seem to be not enough for most your
>> setups.
>
> So there are only two alternatives I can see to set these fields.
> 1. Pass kernel command line parameters, or
> 2. Modify the defaults in ram.c.
>
> Neither of these alternatives is particularly clean to me. The first
> is shunned to avoid clutter on the command line.
> And the second requires us to maintain our own version of ram.c or
> push our defaults on the rest of the community.
>
> We (ok, maybe just 'I') prefer to keep our device specific values in
> the device tree, even though it is not strictly defining hardware in
> the system.
> What do you think of at least keeping the record_size, console_size
> and ftrace_size fields as optional in the ramoops device tree
> specification?
>
> And as a more general question, why should we try not to put
> configuration in the device tree? It seems like a great (and
> portable) place to put this stuff.
> It certainly seems better to have it there than hardwired in the
> kernel or tacked onto the kernel command line.
You have to account for the fact that you may not be able to update the
dtb which may be part of firmware. I can imagine you might want to
change some of the sizes frequently based on what you are doing and
trying to capture. So even if you put in the DT, you need some easy way
to override the settings. This could always be fixed-up by the
bootloader which is what is done for things we don't expect the firmware
to know like initrd address/size and kernel command line. This is why
the prior discussion I mentioned below suggested putting all this in the
/chosen node.
Rob
>
> Thanks,
>
> bryan.
>
> On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov <[email protected]> wrote:
>> On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
>>> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov <[email protected]> wrote:
>>>> On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
>>>>> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>>>>>> When called with a non-zero of_node, fill out a new ramoops_platform_data
>>>>>> with data from the specified Flattened Device Tree node.
>>>>>> Update ramoops documentation with the new FDT interface.
>>>>>> Update devicetree/binding documentation with pstore/ramoops.txt.
>>>>>
>>>>> Thanks for your work, Bryan! There were a few issues, I fixed
>>>>> them myself but I need your confirmation if you're OK w/ all
>>>>> the changes.
>>>> [...]
>>>>> So, the resulting patch is down below. But I have not pushed
>>>>> it out yet, I'm waiting for your sign off. If you're OK with the
>>>>> changes, please reply with
>>>>> 'Signed-off-by: Bryan Freed <[email protected]>'
>>>>
>>>> Bryan, have you had a chance to look into this?
>>>
>>> Whatever happened to this? Olof was also looking at doing a binding
>>> back in Jan 2012:
>>>
>>> http://www.mail-archive.com/[email protected]/msg09905.html
>>
>> I don't know. Bryan's patch looked good, I'd happily apply it. But I still
>> need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
>> behalf (anyone from Chromium project?). Or redo the patch.
>>
>> Thanks,
>>
>> Anton
On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
[...]
> And as a more general question, why should we try not to put
> configuration in the device tree? It seems like a great (and
> portable) place to put this stuff.
> It certainly seems better to have it there than hardwired in the
> kernel or tacked onto the kernel command line.
But then we have two in-kernel APIs to pass kernel parameters? So we'll
have to maintain two ways of passing the options for each driver. That is
hardly a good solution.
If you would like to see a convenient way to pass kernel/module options
via the device tree, I would suggest implementing something like this:
chosen {
kernel-options {
linux,pstore.record-size = 123;
linux,foo = "bar";
};
};
And then let the kernel translate all these to module_param_*().
I am still not sure about placing the options along with devices layout,
but if we go this route, then that is also viable:
pstore-node {
linux,pstore.record-size = 123;
};
And translate "linux,*" this to module_param_*().
How does that sound?
Thanks,
Anton