| Message ID | ec0d6ee7ea79f0e05041ed3e564b0aadb297ef5f.1542107769.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: clkdev: managed clk lookup and provider registrations | expand |
Quoting Matti Vaittinen (2018-11-13 03:55:58) > With MFD devices the clk properties may be contained in MFD (parent) DT > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > subdevice not to MFD device (parent). Add > devm_of_clk_add_hw_provider_parent to tackle this issue. > > Also clkdev registration lacks of managed registration functions and it > seems few drivers do not drop clkdev lookups at exit. Add > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > releasing at exit. Please split this into clkdev and non-clkdev devm functionality. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > Documentation/driver-model/devres.txt | 3 + > drivers/clk/clk.c | 28 ++++++-- > drivers/clk/clkdev.c | 122 ++++++++++++++++++++++++++-------- > include/linux/clk-provider.h | 11 +++ > include/linux/clkdev.h | 4 ++ > 5 files changed, 136 insertions(+), 32 deletions(-) > > diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt > index 43681ca0837f..fac63760b01c 100644 > --- a/Documentation/driver-model/devres.txt > +++ b/Documentation/driver-model/devres.txt > @@ -238,6 +238,9 @@ CLOCK > devm_clk_put() > devm_clk_hw_register() > devm_of_clk_add_hw_provider() > + devm_of_clk_add_parent_hw_provider() > + devm_clk_hw_register_clkdev() > + devm_clk_release_clkdev() The 'release' or non-common functions shouldn't be documented here. > > DMA > dmaenginem_async_device_register() > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index af011974d4ec..9bb921eb90f6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > of_clk_del_provider(*(struct device_node **)res); > } > > -int devm_of_clk_add_hw_provider(struct device *dev, > +static int __devm_of_clk_add_hw_provider(struct device *dev, > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > void *data), > - void *data) > + struct device_node *of_node, void *data) > { > - struct device_node **ptr, *np; > + struct device_node **ptr; > int ret; > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > if (!ptr) > return -ENOMEM; > > - np = dev->of_node; > - ret = of_clk_add_hw_provider(np, get, data); > + *ptr = of_node; > + ret = of_clk_add_hw_provider(of_node, get, data); > if (!ret) { > - *ptr = np; Why is this moved outside of the if condition? In fact, why isn't just the first line in this hunk deleted and passed to this function as struct device_node *np? > devres_add(dev, ptr); > } else { > devres_free(ptr); > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > return ret; > } Nitpick: Add a newline here. > +int devm_of_clk_add_hw_provider(struct device *dev, > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > + void *data), > + void *data) > +{ > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > +} > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > + void *data), > + void *data) > +{ > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, I'm wondering if we can somehow auto-detect this in devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. If it isn't there, then we go to the parent node and look for a #clock-cells property there in the DT node for that device. Does that make sense? Then there isn't any new API and we can attach the lifetime of the devm registration to the presence of the property indicating this is a clk controller or not. > + data); > +} > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); Can we get some kernel doc on these functions? > + > /** > * of_clk_del_provider() - Remove a previously registered clock provider > * @np: Device node pointer associated with clock provider > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 9ab3db8b3988..f6100b6e06fd 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c [...] > + > +/** > + * devm_clk_release_clkdev - Resource managed clkdev lookup release > + * @dev: device this lookup is bound > + * @con_id: connection ID string on device > + * @dev_id: format string describing device name > + * > + * Drop the clkdev lookup created with devm_clk_hw_register_clkdev. > + * Normally this function will not need to be called and the resource > + * management code will ensure that the resource is freed. > + */ > +void devm_clk_release_clkdev(struct device *dev, const char *con_id, > + const char *dev_id) > +{ > + struct clk_lookup *cl; > + int rval; > + > + cl = clk_find(dev_id, con_id); > + WARN_ON(!cl); > + rval = devres_release(dev, devm_clkdev_release, > + &devm_clk_match_clkdev, cl); Nitpick: Drop & on functions taken as pointers. > + WARN_ON(rval); > +} > +EXPORT_SYMBOL(devm_clk_release_clkdev);
Hello Stephen, Thanks a bunch for taking the time and reviewing this! On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > With MFD devices the clk properties may be contained in MFD (parent) DT > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > subdevice not to MFD device (parent). Add > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > Also clkdev registration lacks of managed registration functions and it > > seems few drivers do not drop clkdev lookups at exit. Add > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > releasing at exit. > > Please split this into clkdev and non-clkdev devm functionality. Allright. I'll split this to two patches. > > --- a/Documentation/driver-model/devres.txt > > +++ b/Documentation/driver-model/devres.txt > > @@ -238,6 +238,9 @@ CLOCK > > devm_clk_put() > > devm_clk_hw_register() > > devm_of_clk_add_hw_provider() > > + devm_of_clk_add_parent_hw_provider() > > + devm_clk_hw_register_clkdev() > > + devm_clk_release_clkdev() > > The 'release' or non-common functions shouldn't be documented here. So I will drop the line mentioning devm_clk_release_clkdev() > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index af011974d4ec..9bb921eb90f6 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > > of_clk_del_provider(*(struct device_node **)res); > > } > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > void *data), > > - void *data) > > + struct device_node *of_node, void *data) > > { > > - struct device_node **ptr, *np; > > + struct device_node **ptr; > > int ret; > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > if (!ptr) > > return -ENOMEM; > > > > - np = dev->of_node; > > - ret = of_clk_add_hw_provider(np, get, data); > > + *ptr = of_node; > > + ret = of_clk_add_hw_provider(of_node, get, data); > > if (!ret) { > > - *ptr = np; > > Why is this moved outside of the if condition? I completely removed the local variable np and just unconditionally set the allocated devres to point at the node (if allocation succeeded). We could of course only do this if the provider registration succeeded and save one assignment - but I guess I intended to remove the curly braces and thus decided to go for one liner after if. But apparently I didn't remove the braces O_o. Well, I can put the assignment inside the condition if you prefer that. > In fact, why isn't just > the first line in this hunk deleted and passed to this function as > struct device_node *np? I am sorry but I don't quite follow your suggestion here. Do you mean we could just pass the struct device_node *np in devres_add()? I thought the pointer passed to devress_add() should be allocated using devres_alloc. Can you please elaborate what you mean? > > > devres_add(dev, ptr); > > } else { > > devres_free(ptr); > > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > > return ret; > > } > > Nitpick: Add a newline here. Will do. > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > + void *data), > > + void *data) > > +{ > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > +} > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > + void *data), > > + void *data) > > +{ > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > I'm wondering if we can somehow auto-detect this in > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > If it isn't there, then we go to the parent node and look for a > #clock-cells property there in the DT node for that device. Does that > make sense? Then there isn't any new API and we can attach the lifetime > of the devm registration to the presence of the property indicating this > is a clk controller or not. Huh. I don't know why but building this kind of logic in core is a bit scary to me. I guess I can try implementing something like this - but I am not really a fan of this. (Accidentally) omit the #clock-cells from node and we go to parent node - I am a novice on this area but this sounds like a potential hazard to me. I believe the driver should know if it's properties should be in own or parent node - and if they are not, then there should be no guessing but error. The lifetime is topic where I would like to get information from you who know the kernel better than I do =) But I guess the parent node is there at least as long as the child device is alive. So for me the life time of get-callback is more crucial - but as I said, I don't understand the kernel in details so you probably know it better than me. But please let me know your final take on this and I will follow the guidance =) > > > + data); > > +} > > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); > > Can we get some kernel doc on these functions? Sure. I will add the doc. Reason why I didn't do it is that the current devm_of_clk_add_hw_provider() did not have doc. But I'll add that in next patch. > > + rval = devres_release(dev, devm_clkdev_release, > > + &devm_clk_match_clkdev, cl); > > Nitpick: Drop & on functions taken as pointers. Ok. Will do.
On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > Hello Stephen, > > Thanks a bunch for taking the time and reviewing this! > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > subdevice not to MFD device (parent). Add > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > Also clkdev registration lacks of managed registration functions and it > > > seems few drivers do not drop clkdev lookups at exit. Add > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > releasing at exit. > > > > Please split this into clkdev and non-clkdev devm functionality. > Allright. I'll split this to two patches. Definitely, please note that clkdev is separate from the common clk stuff, and is actually maintained by a different person (me) although I currently defer much of the review to the common clk guys.
On Fri, Nov 30, 2018 at 11:06:53AM +0000, Russell King - ARM Linux wrote: > On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > > Hello Stephen, > > > > Thanks a bunch for taking the time and reviewing this! > > > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > > subdevice not to MFD device (parent). Add > > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > > > Also clkdev registration lacks of managed registration functions and it > > > > seems few drivers do not drop clkdev lookups at exit. Add > > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > > releasing at exit. > > > > > > Please split this into clkdev and non-clkdev devm functionality. > > Allright. I'll split this to two patches. > > Definitely, please note that clkdev is separate from the common > clk stuff, and is actually maintained by a different person (me) > although I currently defer much of the review to the common clk > guys. Right. This clarifies how clkdev is maintained. Thanks for letting me know. Is it still Ok to do these changes through linux-clk tree or is there some other tree where the devm portion should be submitted? If so, how should the dependencies be handled? Br, Matti Vaittinen > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
Hello Stephen & All, I created v5 where I fixed obvious issues. I'll send it in few minutes. Please note following topics: On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > void *data), > > > - void *data) > > > + struct device_node *of_node, void *data) > > > { > > > - struct device_node **ptr, *np; > > > + struct device_node **ptr; > > > int ret; > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > if (!ptr) > > > return -ENOMEM; > > > > > > - np = dev->of_node; > > > - ret = of_clk_add_hw_provider(np, get, data); > > > + *ptr = of_node; > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > if (!ret) { > > > - *ptr = np; > > > > Why is this moved outside of the if condition? > I completely removed the local variable np and just unconditionally set > the allocated devres to point at the node (if allocation succeeded). We > could of course only do this if the provider registration succeeded and > save one assignment - but I guess I intended to remove the curly braces > and thus decided to go for one liner after if. But apparently I didn't > remove the braces O_o. Well, I can put the assignment inside the > condition if you prefer that. > > > In fact, why isn't just > > the first line in this hunk deleted and passed to this function as > > struct device_node *np? > > I am sorry but I don't quite follow your suggestion here. Do you mean we > could just pass the struct device_node *np in devres_add()? I thought > the pointer passed to devress_add() should be allocated using > devres_alloc. Can you please elaborate what you mean? I could not really spot what to fix in patched code (see below). static int __devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), struct device_node *of_node, void *data) { struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), GFP_KERNEL); if (!ptr) return -ENOMEM; *ptr = of_node; ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) devres_add(dev, ptr); else devres_free(ptr); return ret; } As far as I understand we need to allocate the ptr using devres_alloc. We also need to pass this ptr to of_clk_add_hw_provider - and we must assign our node to the *ptr. (I removed the extra braces - this change is laso included in v5 but I don't see how we should improve). Can you please explain me if you still wish to me change this further? > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > I'm wondering if we can somehow auto-detect this in > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > If it isn't there, then we go to the parent node and look for a > > #clock-cells property there in the DT node for that device. Does that > > make sense? Then there isn't any new API and we can attach the lifetime > > of the devm registration to the presence of the property indicating this > > is a clk controller or not. > > Huh. I don't know why but building this kind of logic in core is a bit > scary to me. I guess I can try implementing something like this - but I > am not really a fan of this. (Accidentally) omit the #clock-cells from > node and we go to parent node - I am a novice on this area but this > sounds like a potential hazard to me. I believe the driver should know > if it's properties should be in own or parent node - and if they are > not, then there should be no guessing but error. The lifetime is topic > where I would like to get information from you who know the kernel > better than I do =) But I guess the parent node is there at least as > long as the child device is alive. So for me the life time of > get-callback is more crucial - but as I said, I don't understand the > kernel in details so you probably know it better than me. But please let > me know your final take on this and I will follow the guidance =) I did not put the 'auto-detection' for provider node in the patch v5 as it really gives me bad vibes :) Maybe it is just my pessimistic nature but I do expect that problems will arise when we accidentally end up in parent node when this is not the purpose. I would rather keep this simple by adding one specific API function more - and keeping the existing API specific as well. But I can do v5 if you insist on having this auto-detection.
Quoting Matti Vaittinen (2018-11-30 02:50:22) > Hello Stephen, > > Thanks a bunch for taking the time and reviewing this! > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > subdevice not to MFD device (parent). Add > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > Also clkdev registration lacks of managed registration functions and it > > > seems few drivers do not drop clkdev lookups at exit. Add > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > releasing at exit. > > > > Please split this into clkdev and non-clkdev devm functionality. > Allright. I'll split this to two patches. > > > > --- a/Documentation/driver-model/devres.txt > > > +++ b/Documentation/driver-model/devres.txt > > > @@ -238,6 +238,9 @@ CLOCK > > > devm_clk_put() > > > devm_clk_hw_register() > > > devm_of_clk_add_hw_provider() > > > + devm_of_clk_add_parent_hw_provider() > > > + devm_clk_hw_register_clkdev() > > > + devm_clk_release_clkdev() > > > > The 'release' or non-common functions shouldn't be documented here. > So I will drop the line mentioning devm_clk_release_clkdev() > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index af011974d4ec..9bb921eb90f6 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > > > of_clk_del_provider(*(struct device_node **)res); > > > } > > > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > void *data), > > > - void *data) > > > + struct device_node *of_node, void *data) > > > { > > > - struct device_node **ptr, *np; > > > + struct device_node **ptr; > > > int ret; > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > if (!ptr) > > > return -ENOMEM; > > > > > > - np = dev->of_node; > > > - ret = of_clk_add_hw_provider(np, get, data); > > > + *ptr = of_node; > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > if (!ret) { > > > - *ptr = np; > > > > Why is this moved outside of the if condition? > I completely removed the local variable np and just unconditionally set > the allocated devres to point at the node (if allocation succeeded). We > could of course only do this if the provider registration succeeded and > save one assignment - but I guess I intended to remove the curly braces > and thus decided to go for one liner after if. But apparently I didn't > remove the braces O_o. Well, I can put the assignment inside the > condition if you prefer that. > > > In fact, why isn't just > > the first line in this hunk deleted and passed to this function as > > struct device_node *np? > > I am sorry but I don't quite follow your suggestion here. Do you mean we > could just pass the struct device_node *np in devres_add()? I thought > the pointer passed to devress_add() should be allocated using > devres_alloc. Can you please elaborate what you mean? I'm just trying to reduce the diff in the patch. > > > > > > devres_add(dev, ptr); > > > } else { > > > devres_free(ptr); [..] > > > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > > +} > > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > I'm wondering if we can somehow auto-detect this in > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > If it isn't there, then we go to the parent node and look for a > > #clock-cells property there in the DT node for that device. Does that > > make sense? Then there isn't any new API and we can attach the lifetime > > of the devm registration to the presence of the property indicating this > > is a clk controller or not. > > Huh. I don't know why but building this kind of logic in core is a bit > scary to me. I guess I can try implementing something like this - but I > am not really a fan of this. (Accidentally) omit the #clock-cells from > node and we go to parent node - I am a novice on this area but this > sounds like a potential hazard to me. I believe the driver should know > if it's properties should be in own or parent node - and if they are > not, then there should be no guessing but error. The lifetime is topic > where I would like to get information from you who know the kernel > better than I do =) But I guess the parent node is there at least as > long as the child device is alive. So for me the life time of > get-callback is more crucial - but as I said, I don't understand the > kernel in details so you probably know it better than me. But please let > me know your final take on this and I will follow the guidance =) Please do the magic instead of adding another API. It makes things simpler and will work for this case without having to change anything besides of_clk_add_provider(). If the DT doesn't have the #clock-cells property in the node being registered then calling clk_get() will fail for any consumer devices that point to the node with a phandle and clock specifier. I don't expect us to get very far into development if that's the case. Of course, we don't fail in of_clk_add_provider() if there isn't a #clock-cells property in the node, we just happily add the node to the provider list and carry on. I doubt anyone is failing to specify the DT property, but maybe they are, in which case we could keep not failing and just add the node of whatever we're called with originally if neither the parent or the passed node have the #clock-cells property. I wouldn't try to go any higher than one node above the current node and look for a #clock-cells though. If this all still seems scary then don't worry about it, I'll implement it myself.
Hello Again Stephen, I did already send v5 prior to your reply but I will create v6 today based on this discussion. On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-11-30 02:50:22) > > Hello Stephen, > > > > Thanks a bunch for taking the time and reviewing this! > > > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > void *data), > > > > - void *data) > > > > + struct device_node *of_node, void *data) > > > > { > > > > - struct device_node **ptr, *np; > > > > + struct device_node **ptr; > > > > int ret; > > > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > > if (!ptr) > > > > return -ENOMEM; > > > > > > > > - np = dev->of_node; > > > > - ret = of_clk_add_hw_provider(np, get, data); > > > > + *ptr = of_node; > > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > > if (!ret) { > > > > - *ptr = np; > > > > > > Why is this moved outside of the if condition? > > I completely removed the local variable np and just unconditionally set > > the allocated devres to point at the node (if allocation succeeded). We > > could of course only do this if the provider registration succeeded and > > save one assignment - but I guess I intended to remove the curly braces > > and thus decided to go for one liner after if. But apparently I didn't > > remove the braces O_o. Well, I can put the assignment inside the > > condition if you prefer that. > > > > > In fact, why isn't just > > > the first line in this hunk deleted and passed to this function as > > > struct device_node *np? > > > > I am sorry but I don't quite follow your suggestion here. Do you mean we > > could just pass the struct device_node *np in devres_add()? I thought > > the pointer passed to devress_add() should be allocated using > > devres_alloc. Can you please elaborate what you mean? > > I'm just trying to reduce the diff in the patch. Oh, right. I will see how renaming the argument to np would impact to patch size. iActually, I never consider the patch size at all - I have only been concentrating on how the resulting file looks like. It didn't ever cross my mind that patch size matters. But I guess the size of chanes is really meaningfull when the amount of changes is large. > > > > devres_add(dev, ptr); > > > > } else { > > > > devres_free(ptr); > [..] > > > > > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > + void *data), > > > > + void *data) > > > > +{ > > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > > > +} > > > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > + void *data), > > > > + void *data) > > > > +{ > > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > > > I'm wondering if we can somehow auto-detect this in > > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > > If it isn't there, then we go to the parent node and look for a > > > #clock-cells property there in the DT node for that device. Does that > > > make sense? Then there isn't any new API and we can attach the lifetime > > > of the devm registration to the presence of the property indicating this > > > is a clk controller or not. > > > > Huh. I don't know why but building this kind of logic in core is a bit > > scary to me. I guess I can try implementing something like this - but I > > am not really a fan of this. (Accidentally) omit the #clock-cells from > > node and we go to parent node - I am a novice on this area but this > > sounds like a potential hazard to me. I believe the driver should know > > if it's properties should be in own or parent node - and if they are > > not, then there should be no guessing but error. The lifetime is topic > > where I would like to get information from you who know the kernel > > better than I do =) But I guess the parent node is there at least as > > long as the child device is alive. So for me the life time of > > get-callback is more crucial - but as I said, I don't understand the > > kernel in details so you probably know it better than me. But please let > > me know your final take on this and I will follow the guidance =) > > Please do the magic instead of adding another API. It makes things > simpler and will work for this case without having to change anything > besides of_clk_add_provider(). All right. Let's go on this direction then. > If the DT doesn't have the #clock-cells property in the node being > registered then calling clk_get() will fail for any consumer devices > that point to the node with a phandle and clock specifier. I don't > expect us to get very far into development if that's the case. Makes sense. So only potential thing to break is if someone out there has broken DT/driver - where they currently see this failure. Eg. they use node w/o #clock-cells as provider and where they try and fail controlling this clock - but ignore the error (and system just "works" with HW defaults). After this change they may actually succeed in controlling - but do control wrong clock. Not likely scenario (sure happens somewhere) - and it involves already broken design. So I agree with you. Besides, you are the maintainer for clk framework and thus get the most of the rain if **** hits the fan =D > Of course, we don't fail in of_clk_add_provider() if there isn't a > #clock-cells property in the node, we just happily add the node to the > provider list and carry on. I doubt anyone is failing to specify the DT > property, but maybe they are, in which case we could keep not failing > and just add the node of whatever we're called with originally if > neither the parent or the passed node have the #clock-cells property. I > wouldn't try to go any higher than one node above the current node and > look for a #clock-cells though. I think we should use parent device's node, not the paren node in DT, right? But I agree, we should only look "one level up in the chain". > > If this all still seems scary then don't worry about it, I'll implement > it myself. It still is somewhat "scary" - but I really would like to use the devm based provider registration in the bd718x7 driver so I will implement it in this series. The engineer version of the "living on the edge", you know =) Br, Matti Vaittinen
Quoting Matti Vaittinen (2018-12-03 23:13:15) > On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote: > > > If the DT doesn't have the #clock-cells property in the node being > > registered then calling clk_get() will fail for any consumer devices > > that point to the node with a phandle and clock specifier. I don't > > expect us to get very far into development if that's the case. > > Makes sense. So only potential thing to break is if someone out there > has broken DT/driver - where they currently see this failure. Eg. they > use node w/o #clock-cells as provider and where they try and fail > controlling this clock - but ignore the error (and system just "works" > with HW defaults). After this change they may actually succeed in > controlling - but do control wrong clock. > > Not likely scenario (sure happens somewhere) - and it involves already > broken design. So I agree with you. Besides, you are the maintainer for > clk framework and thus get the most of the rain if **** hits the fan =D Yes, exactly. > > > Of course, we don't fail in of_clk_add_provider() if there isn't a > > #clock-cells property in the node, we just happily add the node to the > > provider list and carry on. I doubt anyone is failing to specify the DT > > property, but maybe they are, in which case we could keep not failing > > and just add the node of whatever we're called with originally if > > neither the parent or the passed node have the #clock-cells property. I > > wouldn't try to go any higher than one node above the current node and > > look for a #clock-cells though. > > I think we should use parent device's node, not the paren node in DT, > right? But I agree, we should only look "one level up in the chain". Are these two things different? I'm suggesting looking at device_node::parent and trying to find a #clock-cells property.
Hello Stephen, . > > I think we should use parent device's node, not the paren node in DT, > > right? But I agree, we should only look "one level up in the chain". > Are these two things different? I'm suggesting looking at > device_node::parent and trying to find a #clock-cells property. I thought that MFD sub-devices may completely lack the DT node but I will verify this tomorrow. BTW, I already did the patch version 6 which checks the dev->parent->of_node if #clock-cells is not found from device's own node or if own node is NULL. Br. Matti Vaittinen
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt index 43681ca0837f..fac63760b01c 100644 --- a/Documentation/driver-model/devres.txt +++ b/Documentation/driver-model/devres.txt @@ -238,6 +238,9 @@ CLOCK devm_clk_put() devm_clk_hw_register() devm_of_clk_add_hw_provider() + devm_of_clk_add_parent_hw_provider() + devm_clk_hw_register_clkdev() + devm_clk_release_clkdev() DMA dmaenginem_async_device_register() diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index af011974d4ec..9bb921eb90f6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) of_clk_del_provider(*(struct device_node **)res); } -int devm_of_clk_add_hw_provider(struct device *dev, +static int __devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), - void *data) + struct device_node *of_node, void *data) { - struct device_node **ptr, *np; + struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, if (!ptr) return -ENOMEM; - np = dev->of_node; - ret = of_clk_add_hw_provider(np, get, data); + *ptr = of_node; + ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) { - *ptr = np; devres_add(dev, ptr); } else { devres_free(ptr); @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, return ret; } +int devm_of_clk_add_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); +} EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); +int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, + data); +} +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); + /** * of_clk_del_provider() - Remove a previously registered clock provider * @np: Device node pointer associated with clock provider diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 9ab3db8b3988..f6100b6e06fd 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -401,6 +401,25 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, return cl; } +static int do_clk_register_clkdev(struct clk_hw *hw, + struct clk_lookup **cl, const char *con_id, const char *dev_id) +{ + + if (IS_ERR(hw)) + return PTR_ERR(hw); + /* + * Since dev_id can be NULL, and NULL is handled specially, we must + * pass it as either a NULL format string, or with "%s". + */ + if (dev_id) + *cl = __clk_register_clkdev(hw, con_id, "%s", + dev_id); + else + *cl = __clk_register_clkdev(hw, con_id, NULL); + + return *cl ? 0 : -ENOMEM; +} + /** * clk_register_clkdev - register one clock lookup for a struct clk * @clk: struct clk to associate with all clk_lookups @@ -420,20 +439,10 @@ int clk_register_clkdev(struct clk *clk, const char *con_id, { struct clk_lookup *cl; - if (IS_ERR(clk)) - return PTR_ERR(clk); - - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s", - dev_id); - else - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL); - - return cl ? 0 : -ENOMEM; + if (!IS_ERR(clk)) + return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id, + dev_id); + return PTR_ERR(clk); } EXPORT_SYMBOL(clk_register_clkdev); @@ -456,18 +465,79 @@ int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id, { struct clk_lookup *cl; - if (IS_ERR(hw)) - return PTR_ERR(hw); + return do_clk_register_clkdev(hw, &cl, con_id, dev_id); +} +EXPORT_SYMBOL(clk_hw_register_clkdev); - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(hw, con_id, "%s", dev_id); - else - cl = __clk_register_clkdev(hw, con_id, NULL); +static void devm_clkdev_release(struct device *dev, void *res) +{ + clkdev_drop(*(struct clk_lookup **)res); +} - return cl ? 0 : -ENOMEM; +static int devm_clk_match_clkdev(struct device *dev, void *res, void *data) +{ + struct clk_lookup **l = res; + + if (!l || !*l) { + WARN_ON(!l || !*l); + return 0; + } + return *l == data; } -EXPORT_SYMBOL(clk_hw_register_clkdev); + +/** + * devm_clk_release_clkdev - Resource managed clkdev lookup release + * @dev: device this lookup is bound + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * Drop the clkdev lookup created with devm_clk_hw_register_clkdev. + * Normally this function will not need to be called and the resource + * management code will ensure that the resource is freed. + */ +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id) +{ + struct clk_lookup *cl; + int rval; + + cl = clk_find(dev_id, con_id); + WARN_ON(!cl); + rval = devres_release(dev, devm_clkdev_release, + &devm_clk_match_clkdev, cl); + WARN_ON(rval); +} +EXPORT_SYMBOL(devm_clk_release_clkdev); + +/** + * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw + * @dev: device this lookup is bound + * @hw: struct clk_hw to associate with all clk_lookups + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * con_id or dev_id may be NULL as a wildcard, just as in the rest of + * clkdev. + * + * To make things easier for mass registration, we detect error clk_hws + * from a previous clk_hw_register_*() call, and return the error code for + * those. This is to permit this function to be called immediately + * after clk_hw_register_*(). + */ +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id) +{ + int rval = -ENOMEM; + struct clk_lookup **cl; + + cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); + if (cl) { + rval = do_clk_register_clkdev(hw, cl, con_id, dev_id); + if (!rval) + devres_add(dev, cl); + else + devres_free(cl); + } + return rval; +} +EXPORT_SYMBOL(devm_clk_hw_register_clkdev); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 60c51871b04b..a6663f084cf1 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -916,6 +916,10 @@ int devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), void *data); +int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data); void of_clk_del_provider(struct device_node *np); void devm_of_clk_del_provider(struct device *dev); struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, @@ -953,6 +957,13 @@ static inline int devm_of_clk_add_hw_provider(struct device *dev, { return 0; } +static inline int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return 0; +} static inline void of_clk_del_provider(struct device_node *np) {} static inline void devm_of_clk_del_provider(struct device *dev) {} static inline struct clk *of_clk_src_simple_get( diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h index 4890ff033220..ccb32af5848b 100644 --- a/include/linux/clkdev.h +++ b/include/linux/clkdev.h @@ -52,4 +52,8 @@ int clk_add_alias(const char *, const char *, const char *, struct device *); int clk_register_clkdev(struct clk *, const char *, const char *); int clk_hw_register_clkdev(struct clk_hw *, const char *, const char *); +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id); +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id); #endif
With MFD devices the clk properties may be contained in MFD (parent) DT node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD subdevice not to MFD device (parent). Add devm_of_clk_add_hw_provider_parent to tackle this issue. Also clkdev registration lacks of managed registration functions and it seems few drivers do not drop clkdev lookups at exit. Add devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup releasing at exit. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- Documentation/driver-model/devres.txt | 3 + drivers/clk/clk.c | 28 ++++++-- drivers/clk/clkdev.c | 122 ++++++++++++++++++++++++++-------- include/linux/clk-provider.h | 11 +++ include/linux/clkdev.h | 4 ++ 5 files changed, 136 insertions(+), 32 deletions(-)