[PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs

[lee.jones@linaro.org (Lee Jones)]

Whoa, what a brilliant response. Thanks Arnd.

I think I'm going to have to take this offline with Linus.

On 25/08/11 15:56, Arnd Bergmann wrote:
> On Thursday 25 August 2011, Lee Jones wrote:
>> On 24/08/11 17:10, Arnd Bergmann wrote:
>>> On Wednesday 10 August 2011, Lee Jones wrote:
>>>> +
>>>> +static void ux500_get_soc_id(char *buf)
>>>> +{
>>>> +       void __iomem *uid_base;
>>>> +       int i;
>>>> +       ssize_t sz = 0;
>>>> +
>>>> +       if (dbx500_partnumber() == 0x8500) {
>>>> +               uid_base = __io_address(U8500_BB_UID_BASE);
>>>> +               for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
>>>> +                       sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
>>>> +               }
>>>> +               return;
>>>> +       } else {
>>>> +               /* Don't know where it is located for U5500 */
>>>> +               sprintf(buf, "N/A");
>>>> +               return;
>>>> +       }
>>>> +}
>>>> +
>>>
>>> This still feels like it's hanging upside-down. You add an SOC node before you know
>>> what it is, and then attach other device to it.
>>
>> Does this comment have anything to do with the code above, or was it
>> quoted just to illustrate that we do find out what the SoC is eventually?
> 
> I should have been more elaborate here. The problem is that you try to provide
> a generic *soc*id* function for the entire ux500 platform, which already
> supports multiple SoCs and will gain support for further ones. Obviously,
> the ID is a central part of the SoC that will be different for every one,
> making it totally pointless to share the function across multiple SoCs.
> 
> Then you go further and inside that function check which soc you actually
> have and *directly* access the registers from some magic address constant.
> We spend a lot of work right now trying to get rid of those constants,
> but a lot of the time we still need them to set up platform_devices on
> platforms that don't yet use device tree probing. However, under no
> circumstances should a random function just take a hardcoded base
> address and do I/O on that.
> 
>> If I am understanding you correctly, you mean that we're registering a
>> '/sys/devices/soc/NODE', before we know what kind of SoC we're dealing
>> with and which devices it contains?
>>
>> If that is the case, I don't believe that this is an issue. If this code
>> has been reached, we know that we're dealing with an SoC, of any type
>> and we decided on a naming convention of 1, 2, 3, ..., thus making prior
>> knowledge of the type of SoC irrelevant.
>>
>> IMO, as soon as we know we're dealing with an SoC, then
>> '/sys/devices/soc' along with the first node '1' should be registered.
>> Then as we have devices appear, they should be allowed to register
>> themselves as children of that node.
>>
>> Again, please correct me if I misunderstand you.
> 
> The name is indeed irrelevant, although a name such as 'db8500' would
> arguably be more useful than a name of '1'.
> 
> Why can't you just put all db8500 specific code into the cpu-db8500.c
> file along with the other code that knows about what a db8500 looks like?
> 
>>> Similarly, having a function named 'ux500_soc_sysfs_init' is plain wrong.
>>>
>>> You don't initialize sysfs here, but you should be probing a device with a
>>> driver that happens to have a sysfs interface.
>>
>> Understood. Would something like 'ux500_export_soc_info_init' be more
>> suitable, or maybe even drop the init? It seems a little long (although
>> fully describes what we're trying to achieve) to me. Perhaps you can
>> provide a more succinct suggestion.
> 
> The problem is the idea that you separate the "info" part from the actual
> "soc" part. What I want to see is a *driver* for the soc that handles all
> aspects of the soc that are unique to the soc but not to specific
> devices inside of the soc.
> 
>>> All probing of devices in general should start at the root and then trickle
>>> down as you discover the child devices:
>>> Each board has its own init_machine() callback that knows the main devices,
>>> most importantly the SoC and registers them. When the driver for the SoC
>>> gets loaded, that driver knows what devices are present in the device and
>>> registers those recursively.
>>
>> I completely agree with you. So how does that differ to what's happening
>> here?
> 
> You currently have a single mop500_init_machine() function that tries to handle
> multiple very different boards, instead of having one init_machine function
> for each one that is actually different.
> 
>>> When you get this right, you can also eliminate the ugly machine_is_* checks
>>> in the board file.
>>
>> We can?
> 
> What you should have here is instead of 
> 
>> static void __init mop500_init_machine(void)
>> {
>>        int i2c0_devs;
>>
>>        /*
>>         * The HREFv60 board removed a GPIO expander and routed
>>         * all these GPIO pins to the internal GPIO controller
>>         * instead.
>>         */
>>        if (!machine_is_snowball()) {
>>                if (machine_is_hrefv60())
>>                        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
>>                else
>>                        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
>>        }
>>
>>        u8500_init_devices();
>>
>>        mop500_pins_init();
>>        if (machine_is_snowball())
>>                platform_add_devices(snowball_platform_devs,
>>                                        ARRAY_SIZE(snowball_platform_devs));
>>        else
>>                platform_add_devices(mop500_platform_devs,
>>                                        ARRAY_SIZE(mop500_platform_devs));
> 
> just do
> 
> static void snowball_init_machine(void)
> {
> 	u8500_init_devices();
> 	snowball_pins_init();
> 	platform_add_devices(snowball_platform_devs,
>                              ARRAY_SIZE(snowball_platform_devs));
> 	...
> }
> 
> static void hrefv60_init_machine(void)
> {
> 	u8500_init_devices();
> 	hrefv60_pins_init();
> 	platform_add_devices(mop500_platform_devs,
>                              ARRAY_SIZE(mop500_platform_devs));
> 	...
> }
> 
> Everything related to the soc node should then go into the u8500_init_devices()
> function that already knows how to take care of that soc. The remaining parts
> of the init_machine just deal with the board-specific components, which can
> of course be very similar on related boards, e.g. each of the boards would
> add an ab8500 device to the external bus, if I interpret the source correctly.
> 
> 	Arnd


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog