2) Provide a name recognized by the device_get_binding() function, this
includes what are currently thought to be drivers such as timers, IOAPIC, and
LOAPIC.

Why do you need this for the LOAPIC/IOAPIC?
All its functions are private to the kernel and do not require a device pointer.
Please provide a specific example on where you would need to run device_get_binding() specifically for the APIC.
device_get_binding() is *slow*. It does a linear search + strcmp(). Use it sparingly.

Andrew

On Fri, 2016-03-11 at 19:02 +0000, Kalowsky, Daniel wrote:

-----Original Message-----
From: Boie, Andrew P [mailto:andrew.p.boie(a)intel.com]
Sent: Friday, March 11, 2016 9:24 AM
To: Thomas, Ramesh <ramesh.thomas(a)intel.com>;
devel(a)lists.zephyrproject.org
Subject: [devel] Re: 2/5 System Device Driver Modifications

2) Provide a name recognized by the device_get_binding() function, this
includes what are currently thought to be drivers such as timers, IOAPIC,

and

LOAPIC.

Why do you need this for the LOAPIC/IOAPIC?
All its functions are private to the kernel and do not require a device pointer.
Please provide a specific example on where you would need to run
device_get_binding() specifically for the APIC.
device_get_binding() is *slow*. It does a linear search + strcmp(). Use it
sparingly.

We agree that the device_get_binding is slow. We'd rather not depend upon it,
which is why we had suggested the addition of a routine to get the start and
end of the device list and allow the PMA to parse this list once at
initialization to figure out what is or is not needed for power
policies.

If you just need to iterate over all devices this code does the job:

struct device *info;

for (info = __device_init_start; info != __device_init_end; info++) {
/* do stuff */
}

Could add an API to nanokernel/device.c to take a function(struct device
*d) as a parameter and apply to all of them so that the private
__device_init_* symbols don't need to be public.

If we go with that solution, pushing all policy decisions to the PMA,
we need to be able to properly identify the devices as that list is
parsed. Currently several devices, like IOAPIC and LOAPIC, have no
name or function to identify them and make this route difficult.

I'm still confused. Does the PMA need to only apply .suspend/.resume
operations to a subset of all devices? Some examples/usecases would
help...


--
Andrew Boie
Staff Engineer - EOS Zephyr
Intel Open Source Technology Center

On Fri, 2016-03-11 at 21:03 +0000, Boie, Andrew P wrote:

On Fri, 2016-03-11 at 19:02 +0000, Kalowsky, Daniel wrote:

-----Original Message-----
From: Boie, Andrew P [mailto:andrew.p.boie(a)intel.com]
Sent: Friday, March 11, 2016 9:24 AM
To: Thomas, Ramesh <ramesh.thomas(a)intel.com>;
devel(a)lists.zephyrproject.org
Subject: [devel] Re: 2/5 System Device Driver Modifications

2) Provide a name recognized by the device_get_binding() function, this
includes what are currently thought to be drivers such as timers, IOAPIC,

and

LOAPIC.

Why do you need this for the LOAPIC/IOAPIC?
All its functions are private to the kernel and do not require a device pointer.
Please provide a specific example on where you would need to run
device_get_binding() specifically for the APIC.
device_get_binding() is *slow*. It does a linear search + strcmp(). Use it
sparingly.

We agree that the device_get_binding is slow. We'd rather not depend upon it,
which is why we had suggested the addition of a routine to get the start and
end of the device list and allow the PMA to parse this list once at
initialization to figure out what is or is not needed for power
policies.

If you just need to iterate over all devices this code does the job:

struct device *info;

for (info = __device_init_start; info != __device_init_end; info++) {
/* do stuff */
}

Could add an API to nanokernel/device.c to take a function(struct device
*d) as a parameter and apply to all of them so that the private
__device_init_* symbols don't need to be public.

The list enables the PMA to choose the order in which it
suspends/resumes the devices by creating its own ordered list based on
policies. If the iteration is done by the kernel then the order will be
fixed to the order the devices are stored in the kernel's device list.

If we go with that solution, pushing all policy decisions to the PMA,
we need to be able to properly identify the devices as that list is
parsed. Currently several devices, like IOAPIC and LOAPIC, have no
name or function to identify them and make this route difficult.

I'm still confused. Does the PMA need to only apply .suspend/.resume
operations to a subset of all devices? Some examples/usecases would
help...

For example, the PMA may chooses to apply .resume on -> APICs, Timers,
ARC, UART in that order because of dependencies and policies. Assuming
it has a list of devices, it can identify these devices in that list by
the name.

The goal is to leave the ordering of operations on devices to the PMA as
kernel can only guarantee the initialization order.

Another reason of giving the PMA direct access to the device is to
handle the case of DEV_BUSY/DEV_FAIL. It can then choose to
retry .suspend only that device that returned DEV_BUSY. Also it can
choose which devices it can ignore a failure and continue suspend.

On Fri, 2016-03-11 at 22:30 +0000, Thomas, Ramesh wrote:

For example, the PMA may chooses to apply .resume on -> APICs, Timers,
ARC, UART in that order because of dependencies and policies. Assuming
it has a list of devices, it can identify these devices in that list by
the name.

The goal is to leave the ordering of operations on devices to the PMA as
kernel can only guarantee the initialization order.

I see. So at boot it could use device_get_binding() or DEVICE_GET() to
populate some data structure with device pointers which is then used at
suspend/resume time.

Another reason of giving the PMA direct access to the device is to
handle the case of DEV_BUSY/DEV_FAIL. It can then choose to
retry .suspend only that device that returned DEV_BUSY.

You're already calling .suspend the first time so presumably you already
have a device pointer, so I'm not sure how this imposes a requirement on
naming.

However the custom ordering for suspend/resume dependencies makes
perfect sense, thanks for the additional detail.

Looking at our headers I see some areas needing improvement. These
anonymous drivers use SYS_INIT(). This macro declares a device with ""
for its name, and the name you would use for DEVICE_GET() is very
counter-intuitive. Which means you can't really use either
device_get_binding() or DEVICE_GET() with them.

I think this API needs to be refactored for system devices like this.
Maybe have SYS_INIT() take a dev_name parameter. Here's a potential way
with a new macro SYS_INIT_NAMED(), the difference is in the first couple
args to DEVICE_INIT():

Old:
#define SYS_INIT(init_fn, level, prio) \
DEVICE_INIT(sys_init_##init_fn, "", init_fn, NULL, NULL, level, prio)

New:
#define SYS_INIT_NAMED(name, init_fn, level, prio) \
DEVICE_INIT(name, STRINGIFY(name), init_fn, NULL, NULL, level, prio)


--
Andrew Boie
Staff Engineer - EOS Zephyr
Intel Open Source Technology Center

On Fri, 2016-03-11 at 23:41 +0000, Boie, Andrew P wrote:

On Fri, 2016-03-11 at 22:30 +0000, Thomas, Ramesh wrote:

For example, the PMA may chooses to apply .resume on -> APICs, Timers,
ARC, UART in that order because of dependencies and policies. Assuming
it has a list of devices, it can identify these devices in that list by
the name.

The goal is to leave the ordering of operations on devices to the PMA as
kernel can only guarantee the initialization order.

I see. So at boot it could use device_get_binding() or DEVICE_GET() to
populate some data structure with device pointers which is then used at
suspend/resume time.

Another reason of giving the PMA direct access to the device is to
handle the case of DEV_BUSY/DEV_FAIL. It can then choose to
retry .suspend only that device that returned DEV_BUSY.

You're already calling .suspend the first time so presumably you already
have a device pointer, so I'm not sure how this imposes a requirement on
naming.

However the custom ordering for suspend/resume dependencies makes
perfect sense, thanks for the additional detail.

Looking at our headers I see some areas needing improvement. These
anonymous drivers use SYS_INIT(). This macro declares a device with ""
for its name, and the name you would use for DEVICE_GET() is very
counter-intuitive. Which means you can't really use either
device_get_binding() or DEVICE_GET() with them.

I think this API needs to be refactored for system devices like this.
Maybe have SYS_INIT() take a dev_name parameter. Here's a potential way
with a new macro SYS_INIT_NAMED(), the difference is in the first couple
args to DEVICE_INIT():

Old:
#define SYS_INIT(init_fn, level, prio) \
DEVICE_INIT(sys_init_##init_fn, "", init_fn, NULL, NULL, level, prio)

New:
#define SYS_INIT_NAMED(name, init_fn, level, prio) \
DEVICE_INIT(name, STRINGIFY(name), init_fn, NULL, NULL, level, prio)

Sounds good. We can use the SYS_INIT_NAMED() macro for devices that
need a name for PM purpose.

Do you think we should replace SYS_INIT() with the new one? i.e. just
modify SYS_INIT(). I am ok either way.

Do you think we should replace SYS_INIT() with the new one? i.e. just
modify SYS_INIT(). I am ok either way.

That was my first thought but I don't think we have the liberty of changing our 1.0 APIs.

Andrew