drivers: clock_control: API extension proposal #16675

nordic-krch · 2019-06-07T09:33:29Z

Proposal which extends API to allow asynchronous clock enabling.

Fixes #11107.
Fixes #12039.

Signed-off-by: Krzysztof Chruscinski [email protected]

include/clock_control.h

nordic-krch · 2019-06-07T09:43:10Z

@pizi-nordic @cvinayak @carlescufi @tbursztyka @masz-nordic
I'm putting following proposal on a table regarding asynchronous enabling of clock. Regarding clock_control_get_status function. There was following proposal #11107 (comment) that i did not included but it may as well replace clock_control_get_status . More important is asynchronous enabling feature.

Only enabling clock is asynchronous. I don't see a need for asynchronous disabling as user usually don't care when clock is disabled (and in many cases it won't be as there might be other users).

Currently Bluetooth implements it's own mechanism to start the HF XTAL clock before radio activity. I've added when parameter to the call since i think that it can be moved to the driver.

nordic-krch · 2019-06-07T10:03:49Z

fyi @joerchan @thomasstenersen

include/clock_control.h

cvinayak · 2019-06-07T10:55:06Z

include/clock_control.h

why not use errno values?

There is no error status here so far.

changed to return:
0 - running
-EALREADY - starting
-ESHUTDOWN - off

I would have kept the original enum. errno are describing errors on calls, not a generic non-erroneous status.

what about clock_control_async_on should that return 0 when running and -EALREADY or -EINPROGRESS when scheduled? It would also be good to clarify error code returned by clock_control_on. Currently it does not specify error codes and nordic driver returns -EINPROGRESS or 0. Others usually return 0 always.

Indeed codes are missing.

In case of clock_control_on it is straightforward:

0, even if it is already on. It's simpler, no need to let the user handling an error which anyway means what he requested was a success (he wanted it on, it is on anyway).

EINVAL if subsystem is bogus

In case of clock_control_async_on, since there is a callback you'll need to verify it (on the same subsystem):

0 if it went fine

-EINPROGRESS if given cb/user_data are the same as running one

-EBUSY is cb/user_data are different

-ENOTSUP some clock controller are immediate, so such function won't be implemented for them. Am I doing the right assumption here? ([edit] seems I did yes)

-EINVAL if cb is not provided (seems mandatory to have one)

0 if it went fine

do you mean that it's up and running or started? I'm still not sure if we should distinguish between started and starting state here. I see following use cases for distinction:

User can use that information is to determine from which context callback was called.

user can start clock in blocking manner while (clock_control_async_on(dev, NULL, now)==-EINPROGRESS));

-EBUSY i would rather return that if provided user_data is already in the list which is rather application error.

-EINVAL if cb is not provided (seems mandatory to have one)

I don't think it has to be mandatory. This could be supported and once driver implements it then counter_clock_on() can be simply counter_clock_async_on(dev, NULL, now).

Good point about -EINPROGRESS, let's not distinguish started/running.
As well as for the -EINVAL removal, I did not see that possibility of clock_on pointing to async_on.

enum is brought back. It contains unknown status which means lack of support for checking status.

tbursztyka · 2019-06-07T12:56:14Z

include/clock_control.h

I really preferred the enum in case get_status is implemented. -ENOTSUP is fine as an error, rest are just non erroneous status code.

is it ok to return positive values as enum values and negative as error? I would prefer to keep function returning through value. It can be that enum will have status unknown which will mean that getting status is not supported.

it is solved as described in #16675 (comment)

pizi-nordic · 2019-06-11T07:41:10Z

include/clock_control.h

Usually we have XXX_DEFINE() and XXX_INITIALIZER() macro for such structures.

pizi-nordic · 2019-06-11T08:33:56Z

include/clock_control.h

Must not be on stack or must be in persistent memory? How long the pointer must point to valid memory?

Fixed the description.

pizi-nordic · 2019-06-11T08:34:27Z

include/clock_control.h

Would use different name than timeout, which suggests giving up after the timeout.
BTW. In which half?

i mentioned once deadline instead of timeout (#16675 (comment)). @tbursztyka what do you think? To me deadline sounds better since as a user i want clock to be running at latest at that moment.

In which half?

I've clarified it. Please take another look.

zephyrbot · 2019-06-11T11:23:24Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

carlescufi · 2019-06-11T12:36:18Z

@erwango @MaureenHelm please take a look and give feedback

include/clock_control.h

nordic-krch · 2019-06-17T10:40:31Z

@cvinayak @pizi-nordic @tbursztyka could you check?
@MaureenHelm @erwango can you also take a look?

Wait for comments from others

pizi-nordic · 2019-06-17T14:58:52Z

include/clock_control.h

How you are going to implement "deadline" feature for Nordic?
Also, if this is mandatory feature, how other platforms are supposed to handle this?

On Nordic I plan to use PPI and RTC1 channel (system clock source).
The simplest way for handling that on any platform is to start clock immediately. That covers functionality since deadline is met. It's only about low power. Platforms focused on lower power will find the way. One way is to use system timer adjusted to system latency (e.g. starting clock N ms earlier than requested).

So Clock Control will be bound to system clock.

I have withdrawn deadline argument from async on. In offline discussions we agreed with @pizi-nordic and @carlescufi that it will be difficult to implement it on other platforms than nordic. It would be in fact public api which would be nordic specific for now.

Ideally, we should have kernel API like k_uptime_us_get_32. Absolute, monotonic microseconds counter which could be used for deadline.

Proposal which extends api to allow asynchronous clock enabling. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch · 2019-06-19T05:55:03Z

@pizi-nordic @tbursztyka @cvinayak can you take another look. I think that current patch is quite straightforward as i removed the only thing that was considered controversial - delayed asynchronous request.

nordic-krch · 2019-06-24T14:29:26Z

@cvinayak @tbursztyka ping

stale

carlescufi · 2019-06-25T14:29:09Z

This has now been opened and under review for more than two weeks, with no objections raised. Merging.

nordic-krch added RFC Request For Comments: want input from the community area: Clock Control labels Jun 7, 2019

tbursztyka previously requested changes Jun 7, 2019

View reviewed changes

include/clock_control.h Outdated Show resolved Hide resolved

include/clock_control.h Outdated Show resolved Hide resolved

include/clock_control.h Outdated Show resolved Hide resolved

include/clock_control.h Outdated Show resolved Hide resolved

nordic-krch added the area: API Changes to public APIs label Jun 7, 2019

nordic-krch requested review from MaureenHelm, anangl, cvinayak, erwango and pizi-nordic June 7, 2019 10:06

cvinayak requested changes Jun 7, 2019

View reviewed changes

nordic-krch force-pushed the clock_control_api_proposal branch from 12c25f3 to c646678 Compare June 7, 2019 12:13

tbursztyka reviewed Jun 7, 2019

View reviewed changes

nordic-krch force-pushed the clock_control_api_proposal branch from c646678 to c51cb27 Compare June 10, 2019 06:18

nordic-krch changed the title ~~[RFC] drivers: clock_control: API extension proposal~~ drivers: clock_control: API extension proposal Jun 10, 2019

nordic-krch removed the RFC Request For Comments: want input from the community label Jun 10, 2019

pizi-nordic reviewed Jun 11, 2019

View reviewed changes

nordic-krch force-pushed the clock_control_api_proposal branch from c51cb27 to d271132 Compare June 11, 2019 11:07

nordic-krch force-pushed the clock_control_api_proposal branch 2 times, most recently from 8752585 to f295197 Compare June 11, 2019 12:04

nordic-krch force-pushed the clock_control_api_proposal branch 2 times, most recently from c0d3bc2 to 615e97f Compare June 12, 2019 10:47

cvinayak previously requested changes Jun 12, 2019

View reviewed changes

nordic-krch force-pushed the clock_control_api_proposal branch from 615e97f to a17cce3 Compare June 13, 2019 06:15

cvinayak self-requested a review June 17, 2019 11:33

pizi-nordic reviewed Jun 17, 2019

View reviewed changes

drivers: clock_control: API extension proposal

d4aaffa

Proposal which extends api to allow asynchronous clock enabling. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch force-pushed the clock_control_api_proposal branch from a17cce3 to d4aaffa Compare June 19, 2019 05:49

pizi-nordic approved these changes Jun 24, 2019

View reviewed changes

carlescufi approved these changes Jun 25, 2019

View reviewed changes

cvinayak approved these changes Jun 25, 2019

View reviewed changes

carlescufi merged commit e0943cf into zephyrproject-rtos:master Jun 25, 2019

drivers: clock_control: API extension proposal #16675

drivers: clock_control: API extension proposal #16675

Uh oh!

Conversation

nordic-krch commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nordic-krch commented Jun 7, 2019

Uh oh!

nordic-krch commented Jun 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbursztyka Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zephyrbot commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Jun 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nordic-krch commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nordic-krch commented Jun 7, 2019 •

edited

Loading

tbursztyka Jun 7, 2019 •

edited

Loading

zephyrbot commented Jun 11, 2019 •

edited

Loading