| Message ID | 20240318110842.41956-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add clk_poll_disable_unprepare() | expand |
Hi Biju, On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver > dependent. So make this part clear in API documentation. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. s/driver\K/ and hardware/ Same in the first chunk actually. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. I'd rephrase this, taking other users into account: There's no guarantee that the clock stops within a particular time window or at all, depending on other users of the clock as well as the driver and hardware implementation. > * > * May be called from atomic contexts. > *
Hi Biju, Thank you for the patch. On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver s/synchoronous/synchronous/ > dependent. So make this part clear in API documentation. You need to explain the rationale here, why asynchronous behaviour is preferred. > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. If synchronous operation is not guaranteed, then it's asynchonous. Asynchronous doesn't mean slow, even an asynchronous provider can complete the disable operation before the function returns to the caller. All it means is that there's no guarantee of synchronous operation. I would document it as such: * This function is asynchronous, if may return before the clock provider * completes the unprepare operation. However, below you're addressing the disable operation. Did you mean to patch the documentation for clk_prepare() instead ? Making clk_unprepare() asynchronous seems a bit weird, given that the function may sleep and is expected to take more time. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. As this is the documentation that targets clock providers, I would expand it and explain why a provider may want to make the disable operation not synchronous. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not .is_enabled() should become mandatory if .disable() is not synchronous. The relationship between the two operations should be better explained. > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. This is the documentation of clk_bulk_disable(), you should address clk_disable() too. I've just noticed that both functions are documented in two places, in include/linux/clk.h, and in drivers/clk/. I wonder why that is. It sounds like it should be fixed, or you'll have to patch both documentation blocks. There's another issue that I'll raise in the review of 2/3. > * > * May be called from atomic contexts. > *
Quoting Biju Das (2024-03-18 04:08:40) > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver > dependent. So make this part clear in API documentation. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- I'd like Russell's ack on this.
On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver > dependent. So make this part clear in API documentation. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. What is important is that: clk_unprepare(clk); clk_prepare(clk); must result in the prepare operation being completed before clk_prepare() returns. The same applies to clk_disable(clk) followed by clk_enable(clk). Since these comment updates are aimed at the unprepare/disable functionality, I think they're fine. Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 25371c91a58f..f5fa91a339d7 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) * if the operation may sleep. One example is a clk which is accessed over * I2c. In the complex case a clk gate operation may require a fast and a slow * part. It is this reason that clk_unprepare and clk_disable are not mutually - * exclusive. In fact clk_disable must be called before clk_unprepare. + * exclusive. In fact clk_disable must be called before clk_unprepare. The + * synchronous or asynchronous clock gating operation is driver dependent. */ void clk_unprepare(struct clk *clk) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 4a537260f655..5b493024e1ec 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -113,7 +113,8 @@ struct clk_duty { * sleep. * * @disable: Disable the clock atomically. Called with enable_lock held. - * This function must not sleep. + * This function must not sleep. The synchronous or asynchronous + * disabling of the clock is driver dependent. * * @is_enabled: Queries the hardware to determine if the clock is enabled. * This function must not sleep. Optional, if this op is not diff --git a/include/linux/clk.h b/include/linux/clk.h index 00623f4de5e1..84b02518791f 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, * @clk: clock source * * Inform the system that a clock source is no longer required by - * a driver and may be shut down. + * a driver and may be shut down. It is not guaranteed to ever actually + * be stopped, that will be driver dependent. * * May be called from atomic contexts. *
The API's related to clk disable operation does not explicitly states the synchoronous or asynchrous behaviour as it is driver dependent. So make this part clear in API documentation. Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * No change. v2: * New patch. --- drivers/clk/clk.c | 3 ++- include/linux/clk-provider.h | 3 ++- include/linux/clk.h | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-)