diff mbox

[v2] ARM: remove name from machine_desc for DT platforms

Message ID 5283CC3F.7030306@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Herring Nov. 13, 2013, 7 p.m. UTC
On 11/11/2013 11:38 AM, Olof Johansson wrote:
> On Tue, Nov 05, 2013 at 08:34:56AM -0600, Rob Herring wrote:
>> On Mon, Nov 4, 2013 at 5:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Sun, Nov 3, 2013 at 8:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> As part of the effort to ultimately remove struct machine_desc, the
>>>> machine name is not really needed. It is only used for /proc/cpuinfo
>>>> "Hardware" field. Get this information from the DT instead and remove
>>>> the name string in the machine descriptor for DT machine descriptors. The
>>>> model or machine compatible property from the DT is used instead.
>>>
>>> I've obviously missed some discussions because I wasn't aware of the
>>> plan to remove machine_desc. That's neither here not there, but for as
>>> long as the structure still exists it is really useful to discover
>>> which machine_desc the kernel chooses at boot time. If you're going to
>>> remove this, then at the very least I would embed the filename or
>>> something similar into the machine desc so it is available for
>>> debugging. It can go away when the structure is removed.
>>
>> It's a very long term goal to remove it completely, but it is now
>> optional for "simple" platforms with the default machine_desc and
>> default init ptrs. You might want to look at the patches and the WIP
>> branch I posted[1] which removes the machine_desc for highbank.
>> There's some core boilerplate I'd like your opinion on. Also, I'm sure
>> you are aware of the resistance to add a machine_desc to arm64, so I'm
>> looking at this from that perspective as well. I'm sure there will be
>> platforms which look very similar across arm and arm64.
>>
>> On boot, the model or compatible string from the DTB is printed out
>> (that change is actually in my DT arch clean-up series, not this
>> patch). This has made what is printed out on boot consistent across
>> arches using DT. There is no good reason for this to be arch specific.
>> Presumably the compatible string is unique and just as easily grepped
>> for in the kernel source as the machine_desc name is. Adding a
>> filename string here seems like useless bloat to me.
> 
> I think I agree a bit with Grant's concern here.
> 
> The compatible string is unique, but the machine descriptors might not
> be -- a given device tree might match two of them (one more generic,
> one more specific) -- it would be useful to know which one was used. Or,
> if you missed to enable the platform support for your board so it booted
> with the generic descriptor, but you still needed your own.

I would push back on anyone having multiple machine descriptors for that
purpose.

> Matching it back to which devicetree the kernel was booted with shouldn't be
> hard, I agree -- but knowing which machine entry was picked at boot time
> is still useful information.

We need to separate what we print on boot vs. what is in /proc/cpuinfo.

One issue I'm trying to solve here is customers complain that Midway
systems print "Calxeda Highbank" in /proc/cpuinfo. I don't think adding
a machine desc only for adding a string for Midway would be right. Also,
I'm close to removing the highbank machine descriptor altogether and
printing out "Generic DT" from the default machine desc is not
particularly useful. Do we at least have agreement that the "right"
thing to print in cpuinfo is the model or compatible from DT?

What is printed on boot is already changed to be based on DT, but that
seems to be what you and Grant care about. Does something like this work
for you? This will print either "Machine model: model string (matched
compat)" or "Machine model: 1st compat str (matched compat)"

Rob


@@ -652,6 +653,7 @@ const void * __init of_flat_dt_match_machine(const
void *default_match,
        while ((data = get_next_compat(&compat))) {
                score = of_flat_dt_match(dt_root, compat);
                if (score > 0 && score < best_score) {
+                       best_compat = compat[score - 1];
                        best_data = data;
                        best_score = score;
                }
@@ -674,7 +676,7 @@ const void * __init of_flat_dt_match_machine(const
void *default_match,
                return NULL;
        }

-       pr_info("Machine model: %s\n", of_flat_dt_get_machine_name());
+       pr_info("Machine model: %s (matched %s)\n",
of_flat_dt_get_machine_name(), best_compat);

        return best_data;
 }

Comments

Olof Johansson Nov. 13, 2013, 7:21 p.m. UTC | #1
On Wed, Nov 13, 2013 at 11:00 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 11/11/2013 11:38 AM, Olof Johansson wrote:
>> On Tue, Nov 05, 2013 at 08:34:56AM -0600, Rob Herring wrote:
>>> On Mon, Nov 4, 2013 at 5:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> On Sun, Nov 3, 2013 at 8:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>>
>>>>> As part of the effort to ultimately remove struct machine_desc, the
>>>>> machine name is not really needed. It is only used for /proc/cpuinfo
>>>>> "Hardware" field. Get this information from the DT instead and remove
>>>>> the name string in the machine descriptor for DT machine descriptors. The
>>>>> model or machine compatible property from the DT is used instead.
>>>>
>>>> I've obviously missed some discussions because I wasn't aware of the
>>>> plan to remove machine_desc. That's neither here not there, but for as
>>>> long as the structure still exists it is really useful to discover
>>>> which machine_desc the kernel chooses at boot time. If you're going to
>>>> remove this, then at the very least I would embed the filename or
>>>> something similar into the machine desc so it is available for
>>>> debugging. It can go away when the structure is removed.
>>>
>>> It's a very long term goal to remove it completely, but it is now
>>> optional for "simple" platforms with the default machine_desc and
>>> default init ptrs. You might want to look at the patches and the WIP
>>> branch I posted[1] which removes the machine_desc for highbank.
>>> There's some core boilerplate I'd like your opinion on. Also, I'm sure
>>> you are aware of the resistance to add a machine_desc to arm64, so I'm
>>> looking at this from that perspective as well. I'm sure there will be
>>> platforms which look very similar across arm and arm64.
>>>
>>> On boot, the model or compatible string from the DTB is printed out
>>> (that change is actually in my DT arch clean-up series, not this
>>> patch). This has made what is printed out on boot consistent across
>>> arches using DT. There is no good reason for this to be arch specific.
>>> Presumably the compatible string is unique and just as easily grepped
>>> for in the kernel source as the machine_desc name is. Adding a
>>> filename string here seems like useless bloat to me.
>>
>> I think I agree a bit with Grant's concern here.
>>
>> The compatible string is unique, but the machine descriptors might not
>> be -- a given device tree might match two of them (one more generic,
>> one more specific) -- it would be useful to know which one was used. Or,
>> if you missed to enable the platform support for your board so it booted
>> with the generic descriptor, but you still needed your own.
>
> I would push back on anyone having multiple machine descriptors for that
> purpose.
>
>> Matching it back to which devicetree the kernel was booted with shouldn't be
>> hard, I agree -- but knowing which machine entry was picked at boot time
>> is still useful information.
>
> We need to separate what we print on boot vs. what is in /proc/cpuinfo.
>
> One issue I'm trying to solve here is customers complain that Midway
> systems print "Calxeda Highbank" in /proc/cpuinfo. I don't think adding
> a machine desc only for adding a string for Midway would be right. Also,
> I'm close to removing the highbank machine descriptor altogether and
> printing out "Generic DT" from the default machine desc is not
> particularly useful. Do we at least have agreement that the "right"
> thing to print in cpuinfo is the model or compatible from DT?
>
> What is printed on boot is already changed to be based on DT, but that
> seems to be what you and Grant care about. Does something like this work
> for you? This will print either "Machine model: model string (matched
> compat)" or "Machine model: 1st compat str (matched compat)"


PowerPC prints both platform (which is the equivalent to machine
descriptor name on ARM), _and_ the /model property from the device
tree:


chitra:~# cat /proc/cpuinfo
processor : 0
cpu : PA6T, altivec supported
clock : 500.000000MHz
revision : 1.2 (pvr 0090 0102)

processor : 1
cpu : PA6T, altivec supported
clock : 500.000000MHz
revision : 1.2 (pvr 0090 0102)

timebase : 66666666
platform : PA Semi PWRficient
model : pasemi,chitra
chitra:~#

(Not sure why we chose "pasemi,chitra" instead of a clear-text model
name in this case :-)

Maybe that's the best of both worlds, to add a Model-equivalent entry
in /proc/cpuinfo? Ideally we'd like to rename the "Hardware" one to
"Platform", but since that's an ABI it'll be hard.

Perhaps it's better to use the /model string for Hardware:, and add a
"Machine type" under it? In your case the machine type would
(eventually) be "Generic DT", which should be acceptable.


-Olof
Rob Herring Nov. 13, 2013, 8:04 p.m. UTC | #2
On 11/13/2013 01:21 PM, Olof Johansson wrote:
> On Wed, Nov 13, 2013 at 11:00 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 11/11/2013 11:38 AM, Olof Johansson wrote:
>>> On Tue, Nov 05, 2013 at 08:34:56AM -0600, Rob Herring wrote:
>>>> On Mon, Nov 4, 2013 at 5:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>> On Sun, Nov 3, 2013 at 8:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>>>
>>>>>> As part of the effort to ultimately remove struct machine_desc, the
>>>>>> machine name is not really needed. It is only used for /proc/cpuinfo
>>>>>> "Hardware" field. Get this information from the DT instead and remove
>>>>>> the name string in the machine descriptor for DT machine descriptors. The
>>>>>> model or machine compatible property from the DT is used instead.
>>>>>
>>>>> I've obviously missed some discussions because I wasn't aware of the
>>>>> plan to remove machine_desc. That's neither here not there, but for as
>>>>> long as the structure still exists it is really useful to discover
>>>>> which machine_desc the kernel chooses at boot time. If you're going to
>>>>> remove this, then at the very least I would embed the filename or
>>>>> something similar into the machine desc so it is available for
>>>>> debugging. It can go away when the structure is removed.
>>>>
>>>> It's a very long term goal to remove it completely, but it is now
>>>> optional for "simple" platforms with the default machine_desc and
>>>> default init ptrs. You might want to look at the patches and the WIP
>>>> branch I posted[1] which removes the machine_desc for highbank.
>>>> There's some core boilerplate I'd like your opinion on. Also, I'm sure
>>>> you are aware of the resistance to add a machine_desc to arm64, so I'm
>>>> looking at this from that perspective as well. I'm sure there will be
>>>> platforms which look very similar across arm and arm64.
>>>>
>>>> On boot, the model or compatible string from the DTB is printed out
>>>> (that change is actually in my DT arch clean-up series, not this
>>>> patch). This has made what is printed out on boot consistent across
>>>> arches using DT. There is no good reason for this to be arch specific.
>>>> Presumably the compatible string is unique and just as easily grepped
>>>> for in the kernel source as the machine_desc name is. Adding a
>>>> filename string here seems like useless bloat to me.
>>>
>>> I think I agree a bit with Grant's concern here.
>>>
>>> The compatible string is unique, but the machine descriptors might not
>>> be -- a given device tree might match two of them (one more generic,
>>> one more specific) -- it would be useful to know which one was used. Or,
>>> if you missed to enable the platform support for your board so it booted
>>> with the generic descriptor, but you still needed your own.
>>
>> I would push back on anyone having multiple machine descriptors for that
>> purpose.
>>
>>> Matching it back to which devicetree the kernel was booted with shouldn't be
>>> hard, I agree -- but knowing which machine entry was picked at boot time
>>> is still useful information.
>>
>> We need to separate what we print on boot vs. what is in /proc/cpuinfo.
>>
>> One issue I'm trying to solve here is customers complain that Midway
>> systems print "Calxeda Highbank" in /proc/cpuinfo. I don't think adding
>> a machine desc only for adding a string for Midway would be right. Also,
>> I'm close to removing the highbank machine descriptor altogether and
>> printing out "Generic DT" from the default machine desc is not
>> particularly useful. Do we at least have agreement that the "right"
>> thing to print in cpuinfo is the model or compatible from DT?
>>
>> What is printed on boot is already changed to be based on DT, but that
>> seems to be what you and Grant care about. Does something like this work
>> for you? This will print either "Machine model: model string (matched
>> compat)" or "Machine model: 1st compat str (matched compat)"
> 
> 
> PowerPC prints both platform (which is the equivalent to machine
> descriptor name on ARM), _and_ the /model property from the device
> tree:

IMHO, pointless architectural variation.

> chitra:~# cat /proc/cpuinfo
> processor : 0
> cpu : PA6T, altivec supported
> clock : 500.000000MHz
> revision : 1.2 (pvr 0090 0102)
> 
> processor : 1
> cpu : PA6T, altivec supported
> clock : 500.000000MHz
> revision : 1.2 (pvr 0090 0102)
> 
> timebase : 66666666
> platform : PA Semi PWRficient
> model : pasemi,chitra
> chitra:~#
> 
> (Not sure why we chose "pasemi,chitra" instead of a clear-text model
> name in this case :-)
> 
> Maybe that's the best of both worlds, to add a Model-equivalent entry
> in /proc/cpuinfo? Ideally we'd like to rename the "Hardware" one to
> "Platform", but since that's an ABI it'll be hard.
> 
> Perhaps it's better to use the /model string for Hardware:, and add a
> "Machine type" under it? In your case the machine type would
> (eventually) be "Generic DT", which should be acceptable.

I'm not inclined to expand cpuinfo and prior attempts to add more info
to cpuinfo like SoC name have failed. The differences between x86 and
arm cpuinfo already cause issues for arm. lscpu does not really work on arm.

I'd actually prefer If you really want more system info, then go read
/proc/device-tree.

Rob
Arnd Bergmann Nov. 14, 2013, 12:32 p.m. UTC | #3
On Wednesday 13 November 2013, Rob Herring wrote:
> I'm not inclined to expand cpuinfo and prior attempts to add more info
> to cpuinfo like SoC name have failed. The differences between x86 and
> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.

Well, they turned into drivers/base/soc.c, which adds the information in
a structured way in sysfs, but is only used on a couple of platforms.

> I'd actually prefer If you really want more system info, then go read
> /proc/device-tree.

Good point.

	Arnd
Grant Likely Nov. 14, 2013, 1:33 p.m. UTC | #4
On Thu, Nov 14, 2013 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 13 November 2013, Rob Herring wrote:
>> I'm not inclined to expand cpuinfo and prior attempts to add more info
>> to cpuinfo like SoC name have failed. The differences between x86 and
>> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.
>
> Well, they turned into drivers/base/soc.c, which adds the information in
> a structured way in sysfs, but is only used on a couple of platforms.
>
>> I'd actually prefer If you really want more system info, then go read
>> /proc/device-tree.

I really don't mind pulling that information out of /proc/cpuinfo. My
point is merely that it is incredibly useful to be able to easily find
out exactly which machine_desc got chosen on any given platform. I
don't think I should have to grep the source tree to figure that out.

The kernel log is useful, but it is also helpful to have it in a /proc
or /sysfs file for when the kernel log overflows. /proc/device-tree
doesn't give that information.

g.
Rob Herring Nov. 14, 2013, 2:46 p.m. UTC | #5
On 11/14/2013 07:33 AM, Grant Likely wrote:
> On Thu, Nov 14, 2013 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wednesday 13 November 2013, Rob Herring wrote:
>>> I'm not inclined to expand cpuinfo and prior attempts to add more info
>>> to cpuinfo like SoC name have failed. The differences between x86 and
>>> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.
>>
>> Well, they turned into drivers/base/soc.c, which adds the information in
>> a structured way in sysfs, but is only used on a couple of platforms.
>>
>>> I'd actually prefer If you really want more system info, then go read
>>> /proc/device-tree.
> 
> I really don't mind pulling that information out of /proc/cpuinfo. My
> point is merely that it is incredibly useful to be able to easily find
> out exactly which machine_desc got chosen on any given platform. I
> don't think I should have to grep the source tree to figure that out.
> 
> The kernel log is useful, but it is also helpful to have it in a /proc
> or /sysfs file for when the kernel log overflows. /proc/device-tree
> doesn't give that information.

Either we want to shrink/remove machine_desc or we don't. If we do, then
the name has to go at some point. So what is the reason to remove it
later rather than sooner? Later means we will have "Generic DT based
system" in cpuinfo as platforms transition. Perhaps that is useful to
know in the short term, but once all platforms print the same thing the
information becomes useless. Then we either remove Hardware field from
cpuinfo or change to use DT info. That's changing the string twice
rather than once.

We probably have at least a dozen machine_desc's we are close to being
able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
several sh-mobile). There's also more that probably can be combined
together which will change the string in /proc/cpuinfo.

If this is a required feature then we should make machine_desc common
(or at least part of it) with the other arches that have copied arm and
then add it to arm64 as well. Then I could do further consolidation of
the FDT machine matching code. Otherwise this is all just pointless
architecture variation that really has nothing to do with the architecture.

Rob
Grant Likely Nov. 14, 2013, 5:16 p.m. UTC | #6
On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 11/14/2013 07:33 AM, Grant Likely wrote:
>> On Thu, Nov 14, 2013 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wednesday 13 November 2013, Rob Herring wrote:
>>>> I'm not inclined to expand cpuinfo and prior attempts to add more info
>>>> to cpuinfo like SoC name have failed. The differences between x86 and
>>>> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.
>>>
>>> Well, they turned into drivers/base/soc.c, which adds the information in
>>> a structured way in sysfs, but is only used on a couple of platforms.
>>>
>>>> I'd actually prefer If you really want more system info, then go read
>>>> /proc/device-tree.
>>
>> I really don't mind pulling that information out of /proc/cpuinfo. My
>> point is merely that it is incredibly useful to be able to easily find
>> out exactly which machine_desc got chosen on any given platform. I
>> don't think I should have to grep the source tree to figure that out.
>>
>> The kernel log is useful, but it is also helpful to have it in a /proc
>> or /sysfs file for when the kernel log overflows. /proc/device-tree
>> doesn't give that information.
>
> Either we want to shrink/remove machine_desc or we don't. If we do, then
> the name has to go at some point. So what is the reason to remove it
> later rather than sooner? Later means we will have "Generic DT based
> system" in cpuinfo as platforms transition. Perhaps that is useful to
> know in the short term, but once all platforms print the same thing the
> information becomes useless. Then we either remove Hardware field from
> cpuinfo or change to use DT info. That's changing the string twice
> rather than once.
>
> We probably have at least a dozen machine_desc's we are close to being
> able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
> several sh-mobile). There's also more that probably can be combined
> together which will change the string in /proc/cpuinfo.
>
> If this is a required feature then we should make machine_desc common
> (or at least part of it) with the other arches that have copied arm and
> then add it to arm64 as well. Then I could do further consolidation of
> the FDT machine matching code. Otherwise this is all just pointless
> architecture variation that really has nothing to do with the architecture.

I think you are missing my point. go ahead, take it out of
/proc/cpuinfo. That's not the issue.

However, as long as there is more than one machine desc, the kernel
really needs to report which one was chosen because it reflects the
initialization path. Whether it's done with the .name field or some
other mechanism also doesn't matter either, but I really don't see a
reason to even bother removing .name. It is zero practical cost to
keep it. Removing it in an effort to shrink machine_desc is just silly
because it doesn't have any meaning outside of machine desc. It's the
other fields in that structure that require engineering work to
remove. .name can simply disappear naturally at the same time time
structure is deleted.

g.

>
> Rob
Rob Herring Nov. 14, 2013, 11:17 p.m. UTC | #7
On 11/14/2013 11:16 AM, Grant Likely wrote:
> On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On 11/14/2013 07:33 AM, Grant Likely wrote:
>>> On Thu, Nov 14, 2013 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wednesday 13 November 2013, Rob Herring wrote:
>>>>> I'm not inclined to expand cpuinfo and prior attempts to add more info
>>>>> to cpuinfo like SoC name have failed. The differences between x86 and
>>>>> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.
>>>>
>>>> Well, they turned into drivers/base/soc.c, which adds the information in
>>>> a structured way in sysfs, but is only used on a couple of platforms.
>>>>
>>>>> I'd actually prefer If you really want more system info, then go read
>>>>> /proc/device-tree.
>>>
>>> I really don't mind pulling that information out of /proc/cpuinfo. My
>>> point is merely that it is incredibly useful to be able to easily find
>>> out exactly which machine_desc got chosen on any given platform. I
>>> don't think I should have to grep the source tree to figure that out.
>>>
>>> The kernel log is useful, but it is also helpful to have it in a /proc
>>> or /sysfs file for when the kernel log overflows. /proc/device-tree
>>> doesn't give that information.
>>
>> Either we want to shrink/remove machine_desc or we don't. If we do, then
>> the name has to go at some point. So what is the reason to remove it
>> later rather than sooner? Later means we will have "Generic DT based
>> system" in cpuinfo as platforms transition. Perhaps that is useful to
>> know in the short term, but once all platforms print the same thing the
>> information becomes useless. Then we either remove Hardware field from
>> cpuinfo or change to use DT info. That's changing the string twice
>> rather than once.
>>
>> We probably have at least a dozen machine_desc's we are close to being
>> able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
>> several sh-mobile). There's also more that probably can be combined
>> together which will change the string in /proc/cpuinfo.
>>
>> If this is a required feature then we should make machine_desc common
>> (or at least part of it) with the other arches that have copied arm and
>> then add it to arm64 as well. Then I could do further consolidation of
>> the FDT machine matching code. Otherwise this is all just pointless
>> architecture variation that really has nothing to do with the architecture.
> 
> I think you are missing my point. go ahead, take it out of
> /proc/cpuinfo. That's not the issue.

I understand what you want. I just don't see a solution that satisfies
you, Olof, and me. I don't see an easy way to add both strings in a way
that is not an ABI, but yet is persistently available. I'm not sure I
understand a scenario where you boot successfully enough to read
/proc/cpuinfo, /sys or something, but cannot get the kernel log.

> However, as long as there is more than one machine desc, the kernel
> really needs to report which one was chosen because it reflects the
> initialization path. Whether it's done with the .name field or some
> other mechanism also doesn't matter either, but I really don't see a
> reason to even bother removing .name. It is zero practical cost to
> keep it. Removing it in an effort to shrink machine_desc is just silly
> because it doesn't have any meaning outside of machine desc. It's the
> other fields in that structure that require engineering work to
> remove. .name can simply disappear naturally at the same time time
> structure is deleted.

There is a real problem as I explained with people asking me why Midway
systems report "Calxeda Highbank". My choices were:

1) Change the machine_desc name to be more generic like "Calxeda based"
or something
2) Change the string to accurately reflect Midway vs. Highbank based on
DT and avoid reporting "Generic DT based system" when internal kernel
refactoring occurs.
3) Ignore the question and tell people to go away.

I went with a solution that works with future plans and avoids the same
issue on other platforms (see the BCM name change going into 3.13)
rather than a simple 2 line patch. At this point, I'm going with 3.

BTW, I have done initial engineering work for the other areas which has
seen little feedback[1][2].

Rob

[1] http://www.kernelhub.org/?msg=353670&p=2
[2]
https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/log/?h=highbank-rm-mach-desc
Russell King - ARM Linux Nov. 15, 2013, 7:13 p.m. UTC | #8
On Thu, Nov 14, 2013 at 08:46:14AM -0600, Rob Herring wrote:
> We probably have at least a dozen machine_desc's we are close to being
> able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
> several sh-mobile). There's also more that probably can be combined
> together which will change the string in /proc/cpuinfo.

I hope not (for dove) because it's one of the few ARM platforms I have
here which run a "modern" distribution.  That's rather important,
because I seem to find things that no one else finds, not even Olof's
wonderful build system when running against distro stuff.

It also means that work on sorting out the componentising of DRM stuff
loses a test platform.

Yes, I boot non-DT kernels on the Cubox because its the only way to get
full support there.  Yes, I maintain a separate kernel tree for doing
this but removing non-DT support for Dove will cripple my efforts to
keep that up to date, and continue pushing forward on getting the various
drivers into shape.

However, maybe the whole point _is_ to keep all the fun stuff on these
platforms out of the kernel tree...
Rob Herring Nov. 15, 2013, 8:16 p.m. UTC | #9
On Fri, Nov 15, 2013 at 1:13 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Nov 14, 2013 at 08:46:14AM -0600, Rob Herring wrote:
>> We probably have at least a dozen machine_desc's we are close to being
>> able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
>> several sh-mobile). There's also more that probably can be combined
>> together which will change the string in /proc/cpuinfo.
>
> I hope not (for dove) because it's one of the few ARM platforms I have
> here which run a "modern" distribution.  That's rather important,
> because I seem to find things that no one else finds, not even Olof's
> wonderful build system when running against distro stuff.
>
> It also means that work on sorting out the componentising of DRM stuff
> loses a test platform.
>
> Yes, I boot non-DT kernels on the Cubox because its the only way to get
> full support there.  Yes, I maintain a separate kernel tree for doing
> this but removing non-DT support for Dove will cripple my efforts to
> keep that up to date, and continue pushing forward on getting the various
> drivers into shape.
>
> However, maybe the whole point _is_ to keep all the fun stuff on these
> platforms out of the kernel tree...

I was referring only to DT enabled machine_desc's. I don't think we'll
touch non-DT machine_desc's. An example is combining several per SoC
DT based machine_desc's into a single SoC family machine_desc.
Ideally, we'd get rid of the machine_desc altogether, but that may not
be possible.

Rob
Arnd Bergmann Nov. 15, 2013, 9:05 p.m. UTC | #10
On Friday 15 November 2013, Rob Herring wrote:
> I was referring only to DT enabled machine_desc's. I don't think we'll
> touch non-DT machine_desc's. An example is combining several per SoC
> DT based machine_desc's into a single SoC family machine_desc.
> Ideally, we'd get rid of the machine_desc altogether, but that may not
> be possible.

Right. We may be able to make some fields of the machine_desc
#ifdef CONFIG_ATAGS at some point, but I think we will have legacy
platforms needing machine_desc callbacks for a very long time.

A different question is what to do about new hardware support.
I'm somewhat optimistic that at some point we can mandate that
new machines should no longer require a machine_desc, just
as we already require for ARM64.

	Arnd
Grant Likely Nov. 18, 2013, 12:59 p.m. UTC | #11
On Thu, 14 Nov 2013 17:17:55 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On 11/14/2013 11:16 AM, Grant Likely wrote:
> > On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
> >> On 11/14/2013 07:33 AM, Grant Likely wrote:
> >>> On Thu, Nov 14, 2013 at 9:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>> On Wednesday 13 November 2013, Rob Herring wrote:
> >>>>> I'm not inclined to expand cpuinfo and prior attempts to add more info
> >>>>> to cpuinfo like SoC name have failed. The differences between x86 and
> >>>>> arm cpuinfo already cause issues for arm. lscpu does not really work on arm.
> >>>>
> >>>> Well, they turned into drivers/base/soc.c, which adds the information in
> >>>> a structured way in sysfs, but is only used on a couple of platforms.
> >>>>
> >>>>> I'd actually prefer If you really want more system info, then go read
> >>>>> /proc/device-tree.
> >>>
> >>> I really don't mind pulling that information out of /proc/cpuinfo. My
> >>> point is merely that it is incredibly useful to be able to easily find
> >>> out exactly which machine_desc got chosen on any given platform. I
> >>> don't think I should have to grep the source tree to figure that out.
> >>>
> >>> The kernel log is useful, but it is also helpful to have it in a /proc
> >>> or /sysfs file for when the kernel log overflows. /proc/device-tree
> >>> doesn't give that information.
> >>
> >> Either we want to shrink/remove machine_desc or we don't. If we do, then
> >> the name has to go at some point. So what is the reason to remove it
> >> later rather than sooner? Later means we will have "Generic DT based
> >> system" in cpuinfo as platforms transition. Perhaps that is useful to
> >> know in the short term, but once all platforms print the same thing the
> >> information becomes useless. Then we either remove Hardware field from
> >> cpuinfo or change to use DT info. That's changing the string twice
> >> rather than once.
> >>
> >> We probably have at least a dozen machine_desc's we are close to being
> >> able to remove (sunxi, picoxcell, highbank, dove, rockchip, virt,
> >> several sh-mobile). There's also more that probably can be combined
> >> together which will change the string in /proc/cpuinfo.
> >>
> >> If this is a required feature then we should make machine_desc common
> >> (or at least part of it) with the other arches that have copied arm and
> >> then add it to arm64 as well. Then I could do further consolidation of
> >> the FDT machine matching code. Otherwise this is all just pointless
> >> architecture variation that really has nothing to do with the architecture.
> > 
> > I think you are missing my point. go ahead, take it out of
> > /proc/cpuinfo. That's not the issue.
> 
> I understand what you want. I just don't see a solution that satisfies
> you, Olof, and me. I don't see an easy way to add both strings in a way
> that is not an ABI, but yet is persistently available. I'm not sure I
> understand a scenario where you boot successfully enough to read
> /proc/cpuinfo, /sys or something, but cannot get the kernel log.

Okay, I'm being selfish. When the kernel output overflows the log buf
then I don't get the machine_desc structure. I won't make a big fuss
about this if it causes a lot of pain. As long as it shows up in the
boot log then I'm happy.

g.
Russell King - ARM Linux Nov. 18, 2013, 1:54 p.m. UTC | #12
On Fri, Nov 15, 2013 at 02:16:29AM +0900, Grant Likely wrote:
> On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
> > If this is a required feature then we should make machine_desc common
> > (or at least part of it) with the other arches that have copied arm and
> > then add it to arm64 as well. Then I could do further consolidation of
> > the FDT machine matching code. Otherwise this is all just pointless
> > architecture variation that really has nothing to do with the architecture.
> 
> I think you are missing my point. go ahead, take it out of
> /proc/cpuinfo. That's not the issue.

You do realise this is a user API issue, right?

http://fossies.org/dox/glibc-2.18/ports_2sysdeps_2unix_2sysv_2linux_2arm_2ioperm_8c_source.html

Right, it only has four strings which it matches against, but this is
evidence that /proc/cpuinfo is a user API, and the strings therein are
used by userspace in ways you don't expect.

We can't just go around removing things.
Grant Likely Nov. 19, 2013, 1:43 p.m. UTC | #13
On Mon, Nov 18, 2013 at 1:54 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Nov 15, 2013 at 02:16:29AM +0900, Grant Likely wrote:
>> On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
>> > If this is a required feature then we should make machine_desc common
>> > (or at least part of it) with the other arches that have copied arm and
>> > then add it to arm64 as well. Then I could do further consolidation of
>> > the FDT machine matching code. Otherwise this is all just pointless
>> > architecture variation that really has nothing to do with the architecture.
>>
>> I think you are missing my point. go ahead, take it out of
>> /proc/cpuinfo. That's not the issue.
>
> You do realise this is a user API issue, right?
>
> http://fossies.org/dox/glibc-2.18/ports_2sysdeps_2unix_2sysv_2linux_2arm_2ioperm_8c_source.html
>
> Right, it only has four strings which it matches against, but this is
> evidence that /proc/cpuinfo is a user API, and the strings therein are
> used by userspace in ways you don't expect.
>
> We can't just go around removing things.

Not everything we end up exporting ends up being an ABI. If nobody
uses it (or complains when we remove it in the obscure cases) then
there is no problem :-)

You're absolutely right here though. I strongly doubt the string
matters for the DT platforms, but you're right that it should not be
changed for the old board support.

g.
Rob Herring Nov. 21, 2013, 11:53 p.m. UTC | #14
On Mon, Nov 18, 2013 at 7:54 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Nov 15, 2013 at 02:16:29AM +0900, Grant Likely wrote:
>> On Thu, Nov 14, 2013 at 11:46 PM, Rob Herring <robherring2@gmail.com> wrote:
>> > If this is a required feature then we should make machine_desc common
>> > (or at least part of it) with the other arches that have copied arm and
>> > then add it to arm64 as well. Then I could do further consolidation of
>> > the FDT machine matching code. Otherwise this is all just pointless
>> > architecture variation that really has nothing to do with the architecture.
>>
>> I think you are missing my point. go ahead, take it out of
>> /proc/cpuinfo. That's not the issue.
>
> You do realise this is a user API issue, right?
>
> http://fossies.org/dox/glibc-2.18/ports_2sysdeps_2unix_2sysv_2linux_2arm_2ioperm_8c_source.html
>
> Right, it only has four strings which it matches against, but this is
> evidence that /proc/cpuinfo is a user API, and the strings therein are
> used by userspace in ways you don't expect.
>
> We can't just go around removing things.

I mentioned the string changes in the commit message. If we take the
hard line here, then we can never change these strings and are stuck
with them forever. And in turn, we can never consolidate several board
machine_descs into an SoC machine_desc. However, in the DT case we've
already been changing this string as platforms have been converted to
DT and I don't think anyone has complained. I think we should fix this
sooner rather than later as more DT platforms become entrenched.

Stepping back, where should this information come from in the DT case?
This is an easy thing to have in the DT and not in the kernel. It
seems like a no-brainer to me. Or perhaps we say the Hardware field is
deprecated and remove it on DT? It is already available in
/proc/device-tree.

Rob
diff mbox

Patch

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 5c47910..466b360 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -645,6 +645,7 @@  const void * __init of_flat_dt_match_machine(const
void *default_match,
        const void *data = NULL;
        const void *best_data = default_match;
        const char *const *compat;
+       const char *best_compat = NULL;
        unsigned long dt_root;
        unsigned int best_score = ~1, score = 0;