| Message ID | 20240318110842.41956-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| 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!
Hi Sakari Ailus, Thanks for the feedback. > -----Original Message----- > From: Sakari Ailus <sakari.ailus@linux.intel.com> > Sent: Monday, March 18, 2024 6:29 PM > Subject: Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable > > 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. Driver abstract hardware. But a driver supports different hardware's. One hardware may doesn't care about the sync/async disabling of the clock while another one does. Not sure how other people's thoughts on this change?? > > > * > > * @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. If we elaborate " driver dependent", it matches with your explanation. Not sure we should elaborate it or it is understood from "driver " dependent?? Cheers, Biju > > > * > > * May be called from atomic contexts. > > * > > -- > Kind regards, > > Sakari Ailus
Hi Laurent Pinchart, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Monday, March 18, 2024 10:39 PM > Subject: Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable > > 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/ OK will fix this. > > > dependent. So make this part clear in API documentation. > > You need to explain the rationale here, why asynchronous behaviour is preferred. OK. Because it's not important for functionality and moreover some systems It can enter sleep state quickly and thereby saving power. I will update it in commit message. > > > 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. This patch aimed at the unprepare/disable functionality. > > > */ > > 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. Will explain here as well. > > > * > > * @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. I will update the comment here. > > > 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. This update is for clk_disable(). Am I missing anything? > 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. Sakari also pointed out the same, he suggested to remove it from .c. Cheers, Biju > > There's another issue that I'll raise in the review of 2/3. > > > * > > * May be called from atomic contexts. > > * > > -- > Regards, > > Laurent Pinchart
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(-)