[PATCH 0/6] clk: uclass fixes and improvements
This series fixes a few issues found while writing a clk driver for a new SoC (Bouffalo Lab BL808), and adds the new functionality needed to implement a hierarchy of clocks in a single device (without the CCF). The .get_parent hook will be useful on other platforms as well; I plan to use it to implement PLL rate setting on sunxi.
Samuel Holland (6): clk: Handle error pointers in clk_valid() clk: Fix error handling in clk_get_rate() clk: Fix error handling in clk_get_parent() clk: Fix rate caching in clk_get_parent_rate() clk: Remove an unneeded check from clk_get_parent_rate() clk: Add a .get_parent operation
drivers/clk/clk-uclass.c | 35 ++++++++++++++++------------------- include/clk-uclass.h | 2 ++ include/clk.h | 2 +- 3 files changed, 19 insertions(+), 20 deletions(-)
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org ---
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) { - return clk && !!clk->dev; + return clk && !IS_ERR(clk) && !!clk->dev; }
int soc_clk_dump(void);
On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) {
- return clk && !!clk->dev;
- return clk && !IS_ERR(clk) && !!clk->dev;
}
int soc_clk_dump(void);
Maybe it would be better to avoid the error pointers instead, there should not be that many places where they are returned.
Thanks
Michal
On 2/20/23 05:46, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) {
- return clk && !!clk->dev;
return clk && !IS_ERR(clk) && !!clk->dev; }
int soc_clk_dump(void);
Maybe it would be better to avoid the error pointers instead, there should not be that many places where they are returned.
This not quite the right way to phrase this. Instead, I would ask: where are places where the return value is not checked correctly? It's perfectly fine to pass NULL or uninitialized clocks to other clock functions, but it's not OK to ignore errors.
--Sean
On Mon, Feb 20, 2023 at 10:57:17AM -0500, Sean Anderson wrote:
On 2/20/23 05:46, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) {
- return clk && !!clk->dev;
- return clk && !IS_ERR(clk) && !!clk->dev; } int soc_clk_dump(void);
Maybe it would be better to avoid the error pointers instead, there should not be that many places where they are returned.
This not quite the right way to phrase this. Instead, I would ask: where are places where the return value is not checked correctly? It's perfectly
Pretty much everywhere where clk_get_parent is used outside of core code absolutely no error checking is done.
fine to pass NULL or uninitialized clocks to other clock functions, but it's not OK to ignore errors.
Is there some documentation on what is the distinction, specifically?
If there isn't it's meaningless and NULL can be returned on error simplifying the code, thinking about the code, debugging, pretty much everything.
Thanks
Michal
On 2/20/23 13:42, Michal Suchánek wrote:
On Mon, Feb 20, 2023 at 10:57:17AM -0500, Sean Anderson wrote:
On 2/20/23 05:46, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) {
- return clk && !!clk->dev;
- return clk && !IS_ERR(clk) && !!clk->dev; } int soc_clk_dump(void);
Maybe it would be better to avoid the error pointers instead, there should not be that many places where they are returned.
This not quite the right way to phrase this. Instead, I would ask: where are places where the return value is not checked correctly? It's perfectly
Pretty much everywhere where clk_get_parent is used outside of core code absolutely no error checking is done.
There are 105 uses of ERR_PTR in drivers/clk, mostly in CCF clock registration functions. There is also devm_clk_get() with about 30 callers. Those mostly seem to have reasonable error checking; the error pointer ends up as the driver probe function's return value via PTR_ERR.
fine to pass NULL or uninitialized clocks to other clock functions, but it's not OK to ignore errors.
Is there some documentation on what is the distinction, specifically?
If there isn't it's meaningless and NULL can be returned on error simplifying the code, thinking about the code, debugging, pretty much everything.
What should I do for v2? Keep this patch? Convert clk_get_parent() to return NULL instead of ERR_PTR, and hope nobody uses clk_valid() on the pointer returned from devm_clk_get()?
Regards, Samuel
On Sat, Mar 04, 2023 at 01:54:12PM -0600, Samuel Holland wrote:
On 2/20/23 13:42, Michal Suchánek wrote:
On Mon, Feb 20, 2023 at 10:57:17AM -0500, Sean Anderson wrote:
On 2/20/23 05:46, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
Some clk uclass functions, such as devm_clk_get() and clk_get_parent(), return error pointers. clk_valid() should not consider these pointers to be valid.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
include/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f7..4acb878ec6d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk) */ static inline bool clk_valid(struct clk *clk) {
- return clk && !!clk->dev;
- return clk && !IS_ERR(clk) && !!clk->dev; } int soc_clk_dump(void);
Maybe it would be better to avoid the error pointers instead, there should not be that many places where they are returned.
This not quite the right way to phrase this. Instead, I would ask: where are places where the return value is not checked correctly? It's perfectly
Pretty much everywhere where clk_get_parent is used outside of core code absolutely no error checking is done.
There are 105 uses of ERR_PTR in drivers/clk, mostly in CCF clock registration functions. There is also devm_clk_get() with about 30 callers. Those mostly seem to have reasonable error checking; the error pointer ends up as the driver probe function's return value via PTR_ERR.
Yes, there are too many. clk_valid should check both.
Reviewed-by: Michal Suchánek msuchanek@suse.de
Thanks
Michal
log_ret() cannot work with unsigned values, and the assignment to 'ret' incorrectly truncates the rate from long to int.
Fixes: 5c5992cb90cf ("clk: Add debugging for return values") Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/clk/clk-uclass.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a261..78299dbceb2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -471,7 +471,6 @@ void clk_free(struct clk *clk) ulong clk_get_rate(struct clk *clk) { const struct clk_ops *ops; - int ret;
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk)) @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS;
- ret = ops->get_rate(clk); - if (ret) - return log_ret(ret); - - return 0; + return ops->get_rate(clk); }
struct clk *clk_get_parent(struct clk *clk)
Hello,
On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
log_ret() cannot work with unsigned values, and the assignment to 'ret' incorrectly truncates the rate from long to int.
Fixes: 5c5992cb90cf ("clk: Add debugging for return values") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a261..78299dbceb2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -471,7 +471,6 @@ void clk_free(struct clk *clk) ulong clk_get_rate(struct clk *clk) { const struct clk_ops *ops;
int ret;
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
@@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS;
- ret = ops->get_rate(clk);
- if (ret)
return log_ret(ret);- return 0;
- return ops->get_rate(clk);
}
This is generally poor design of the clock stuff in u-boot.
It returns -ERROR for many error conditions, but also 0 for other error conditions.
Some error handling checks for 0, some for errval, some casts to int and checks for <= 0.
I think that using -ERROR for clocks does not make much sense in u-boot.
Even in the kernel the errval checks are pretty much limited to places where integers are used to store page frame numbers or pointers, that is errptr stored in an integer. For adresses it is pretty easy to make sure that the last page is not mapped making the error pointers invalid (although bugs in that part happened too).
For clocks no such guarantee exists. The only apparently invalid clock is 0, and the correct fix is to fix up the clock code to return 0 on error, always. It's a lot of code to fix, though.
If you do not want to fix everything then the correct thing to do is make ret ulong, and check for errval *and* 0.
There is not much point in returning detailed error codes in u-boot, anyway. It's not like there is some userspace that could interpret them. Most errors are logged when they happen if ever, and callers only check if error happened or not.
Thanks
Michal
On 2/20/23 05:37, Michal Suchánek wrote:
Hello,
On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
log_ret() cannot work with unsigned values, and the assignment to 'ret' incorrectly truncates the rate from long to int.
Fixes: 5c5992cb90cf ("clk: Add debugging for return values") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a261..78299dbceb2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -471,7 +471,6 @@ void clk_free(struct clk *clk) ulong clk_get_rate(struct clk *clk) { const struct clk_ops *ops;
int ret;
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
@@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS;
- ret = ops->get_rate(clk);
- if (ret)
return log_ret(ret);- return 0;
- return ops->get_rate(clk); }
This is generally poor design of the clock stuff in u-boot.
It returns -ERROR for many error conditions, but also 0 for other error conditions.
Some error handling checks for 0, some for errval, some casts to int and checks for <= 0.
I think that using -ERROR for clocks does not make much sense in u-boot.
Even in the kernel the errval checks are pretty much limited to places where integers are used to store page frame numbers or pointers, that is errptr stored in an integer. For adresses it is pretty easy to make sure that the last page is not mapped making the error pointers invalid (although bugs in that part happened too).
For clocks no such guarantee exists. The only apparently invalid clock is 0, and the correct fix is to fix up the clock code to return 0 on error, always. It's a lot of code to fix, though.
If you do not want to fix everything then the correct thing to do is make ret ulong, and check for errval *and* 0.
There is not much point in returning detailed error codes in u-boot, anyway. It's not like there is some userspace that could interpret them. Most errors are logged when they happen if ever, and callers only check if error happened or not.
Thanks
Michal
clk_get_parent is the only place where we return a clock pointer directly. Everywhere else, we have an integer return we can use. This function is different of course because CCF is broken and assumes there is only one canonical struct clk for a logical clock. So it can't initialize a caller-passed struct clk, but instead has to return the one true struct clk.
I agree with Michal here. The signature should really be
int clk_get_parent(struct clk *child, struct clk *parent)
but for now there is no point confusing the rest of the clock subsystem to make it work with error pointers.
--Sean
Hello,
On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
On 2/20/23 05:37, Michal Suchánek wrote:
Hello,
On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
log_ret() cannot work with unsigned values, and the assignment to 'ret' incorrectly truncates the rate from long to int.
Fixes: 5c5992cb90cf ("clk: Add debugging for return values") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a261..78299dbceb2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -471,7 +471,6 @@ void clk_free(struct clk *clk) ulong clk_get_rate(struct clk *clk) { const struct clk_ops *ops;
- int ret; debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
@@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS;
- ret = ops->get_rate(clk);
- if (ret)
return log_ret(ret);- return 0;
- return ops->get_rate(clk); }
This is generally poor design of the clock stuff in u-boot.
It returns -ERROR for many error conditions, but also 0 for other error conditions.
Some error handling checks for 0, some for errval, some casts to int and checks for <= 0.
I think that using -ERROR for clocks does not make much sense in u-boot.
Even in the kernel the errval checks are pretty much limited to places where integers are used to store page frame numbers or pointers, that is errptr stored in an integer. For adresses it is pretty easy to make sure that the last page is not mapped making the error pointers invalid (although bugs in that part happened too).
For clocks no such guarantee exists. The only apparently invalid clock is 0, and the correct fix is to fix up the clock code to return 0 on error, always. It's a lot of code to fix, though.
If you do not want to fix everything then the correct thing to do is make ret ulong, and check for errval *and* 0.
There is not much point in returning detailed error codes in u-boot, anyway. It's not like there is some userspace that could interpret them. Most errors are logged when they happen if ever, and callers only check if error happened or not.
Thanks
Michal
clk_get_parent is the only place where we return a clock pointer directly. Everywhere else, we have an integer return we can use. This function is different of course because CCF is broken and assumes there is only one canonical struct clk for a logical clock. So it can't initialize a caller-passed struct clk, but instead has to return the one true struct clk.
I agree with Michal here. The signature should really be
int clk_get_parent(struct clk *child, struct clk *parent)
but for now there is no point confusing the rest of the clock subsystem to make it work with error pointers.
This is clk_get_rate. It returns a clock rate. A clock rate of 2.5GHz is not out of question, and cannot be expressed with a signed integer.
Hence clock rate should not be treated as signed, and -ERROR should not be returned as clock rate.
Thanks
Michal
On 2/20/23 12:27, Michal Suchánek wrote:
Hello,
On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
On 2/20/23 05:37, Michal Suchánek wrote:
Hello,
On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
log_ret() cannot work with unsigned values, and the assignment to 'ret' incorrectly truncates the rate from long to int.
Fixes: 5c5992cb90cf ("clk: Add debugging for return values") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a261..78299dbceb2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -471,7 +471,6 @@ void clk_free(struct clk *clk) ulong clk_get_rate(struct clk *clk) { const struct clk_ops *ops;
- int ret; debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
@@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk) if (!ops->get_rate) return -ENOSYS;
- ret = ops->get_rate(clk);
- if (ret)
return log_ret(ret);- return 0;
- return ops->get_rate(clk); }
This is generally poor design of the clock stuff in u-boot.
It returns -ERROR for many error conditions, but also 0 for other error conditions.
Some error handling checks for 0, some for errval, some casts to int and checks for <= 0.
I think that using -ERROR for clocks does not make much sense in u-boot.
Even in the kernel the errval checks are pretty much limited to places where integers are used to store page frame numbers or pointers, that is errptr stored in an integer. For adresses it is pretty easy to make sure that the last page is not mapped making the error pointers invalid (although bugs in that part happened too).
For clocks no such guarantee exists. The only apparently invalid clock is 0, and the correct fix is to fix up the clock code to return 0 on error, always. It's a lot of code to fix, though.
If you do not want to fix everything then the correct thing to do is make ret ulong, and check for errval *and* 0.
There is not much point in returning detailed error codes in u-boot, anyway. It's not like there is some userspace that could interpret them. Most errors are logged when they happen if ever, and callers only check if error happened or not.
Thanks
Michal
clk_get_parent is the only place where we return a clock pointer directly. Everywhere else, we have an integer return we can use. This function is different of course because CCF is broken and assumes there is only one canonical struct clk for a logical clock. So it can't initialize a caller-passed struct clk, but instead has to return the one true struct clk.
I agree with Michal here. The signature should really be
int clk_get_parent(struct clk *child, struct clk *parent)
but for now there is no point confusing the rest of the clock subsystem to make it work with error pointers.
This is clk_get_rate.
Ah, whoops. In that case,
Reviewed-by: Sean Anderson seanga2@gmail.com
It returns a clock rate. A clock rate of 2.5GHz is not out of question, and cannot be expressed with a signed integer.
And of course neither is a rate of 5 GHz :)
Hence clock rate should not be treated as signed, and -ERROR should not be returned as clock rate.
Yes...
I would accept a conversion to "0 means unknown rate (for whatever reason)" API.
--Sean
Do not return both NULL and error pointers. The function is only documented as returning error pointers.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/clk/clk-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 78299dbceb2..5bce976b060 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk)) - return NULL; + return ERR_PTR(-ENODEV);
pdev = dev_get_parent(clk->dev); if (!pdev)
On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
Do not return both NULL and error pointers. The function is only documented as returning error pointers.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 78299dbceb2..5bce976b060 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
return NULL;
return ERR_PTR(-ENODEV);pdev = dev_get_parent(clk->dev); if (!pdev)
Do we really need this?
Who cares about the distinction?
Just returning NULL on any error makes the code so much simpler and easier to understand.
Thanks
Michal
On 2/20/23 04:39, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
Do not return both NULL and error pointers. The function is only documented as returning error pointers.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 78299dbceb2..5bce976b060 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
return NULL;
return ERR_PTR(-ENODEV);pdev = dev_get_parent(clk->dev); if (!pdev)
Do we really need this?
Who cares about the distinction?
Just returning NULL on any error makes the code so much simpler and easier to understand.
I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done later in the function, but returning both makes the error handling in callers more complicated for no benefit. It sounds like you would prefer I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I can do that for v2.
Regards, Samuel
On Sat, Mar 04, 2023 at 01:58:17PM -0600, Samuel Holland wrote:
On 2/20/23 04:39, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
Do not return both NULL and error pointers. The function is only documented as returning error pointers.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 78299dbceb2..5bce976b060 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
return NULL;
return ERR_PTR(-ENODEV);pdev = dev_get_parent(clk->dev); if (!pdev)
Do we really need this?
Who cares about the distinction?
Just returning NULL on any error makes the code so much simpler and easier to understand.
I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done later in the function, but returning both makes the error handling in callers more complicated for no benefit. It sounds like you would prefer I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I can do that for v2.
I think NULL is easier to check, and so long as there is no meaningful distinction between ERR_PTR and NULL we should converge towards NULL as the single error return value.
Thanks
Michal
On 3/4/23 15:46, Michal Suchánek wrote:
On Sat, Mar 04, 2023 at 01:58:17PM -0600, Samuel Holland wrote:
On 2/20/23 04:39, Michal Suchánek wrote:
On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
Do not return both NULL and error pointers. The function is only documented as returning error pointers.
Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 78299dbceb2..5bce976b060 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
debug("%s(clk=%p)\n", __func__, clk); if (!clk_valid(clk))
return NULL;
return ERR_PTR(-ENODEV);pdev = dev_get_parent(clk->dev); if (!pdev)
Do we really need this?
Who cares about the distinction?
Just returning NULL on any error makes the code so much simpler and easier to understand.
I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done later in the function, but returning both makes the error handling in callers more complicated for no benefit. It sounds like you would prefer I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I can do that for v2.
I think NULL is easier to check, and so long as there is no meaningful distinction between ERR_PTR and NULL we should converge towards NULL as the single error return value.
The only issue I can see is if you have e.g. an I2C clock and you need to examine the register to determine the parent and the slave doesn't respond. Is the distinction between "I have no parent" and "I have a parent but I don't know which" meaningful? Linux uses one return because parents must be determined at probe time. I'm fine with making this return NULL on error; this should be uncommon enough that we can recommend dev_err() reporting in the driver.
--Sean
clk_get_rate() can return an error value. Recompute the rate if the cached value is an error value.
Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations") Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/clk/clk-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 5bce976b060..9d052e5a814 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk) return -ENOSYS;
/* Read the 'rate' if not already set or if proper flag set*/ - if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) + if (!pclk->rate || IS_ERR_VALUE(pclk->rate) || + pclk->flags & CLK_GET_RATE_NOCACHE) pclk->rate = clk_get_rate(pclk);
return pclk->rate;
On Sun, Feb 19, 2023 at 11:59:37PM -0600, Samuel Holland wrote:
clk_get_rate() can return an error value. Recompute the rate if the cached value is an error value.
Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 5bce976b060..9d052e5a814 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk) return -ENOSYS;
/* Read the 'rate' if not already set or if proper flag set*/
- if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
pclk->flags & CLK_GET_RATE_NOCACHE)pclk->rate = clk_get_rate(pclk);
return pclk->rate;
With the current code that randomly returns one or the other this is the correct thing to do.
Reviewed-by: Michal Suchánek msuchanek@suse.de
Thanks
Michal
On 2/20/23 00:59, Samuel Holland wrote:
clk_get_rate() can return an error value. Recompute the rate if the cached value is an error value.
Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 5bce976b060..9d052e5a814 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk) return -ENOSYS;
/* Read the 'rate' if not already set or if proper flag set*/
- if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
pclk->flags & CLK_GET_RATE_NOCACHE)pclk->rate = clk_get_rate(pclk);
return pclk->rate;
The correct fix here is
/* Read the 'rate' if not already set or if proper flag set*/ if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) { rate = clk_get_rate(pclk); if (!IS_ERR_VALUE(rate)) pclk->rate = rate; }
return rate;
No point caching errors.
--Sean
On 2/20/23 10:11, Sean Anderson wrote:
On 2/20/23 00:59, Samuel Holland wrote:
clk_get_rate() can return an error value. Recompute the rate if the cached value is an error value.
Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations") Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 5bce976b060..9d052e5a814 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk) return -ENOSYS; /* Read the 'rate' if not already set or if proper flag set*/ - if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) + if (!pclk->rate || IS_ERR_VALUE(pclk->rate) || + pclk->flags & CLK_GET_RATE_NOCACHE) pclk->rate = clk_get_rate(pclk); return pclk->rate;
The correct fix here is
/* Read the 'rate' if not already set or if proper flag set*/ if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) { rate = clk_get_rate(pclk); if (!IS_ERR_VALUE(rate)) pclk->rate = rate; }
return rate;
No point caching errors.
Good point. I will do this for v2.
Regards, Samuel
There is no need to check the parent clock's ops. The following call to clk_get_rate() does that already.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/clk/clk-uclass.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 9d052e5a814..53cfd819779 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
ulong clk_get_parent_rate(struct clk *clk) { - const struct clk_ops *ops; struct clk *pclk;
debug("%s(clk=%p)\n", __func__, clk); @@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk) if (IS_ERR(pclk)) return -ENODEV;
- ops = clk_dev_ops(pclk->dev); - if (!ops->get_rate) - return -ENOSYS; - /* Read the 'rate' if not already set or if proper flag set*/ if (!pclk->rate || IS_ERR_VALUE(pclk->rate) || pclk->flags & CLK_GET_RATE_NOCACHE)
On Sun, Feb 19, 2023 at 11:59:38PM -0600, Samuel Holland wrote:
There is no need to check the parent clock's ops. The following call to clk_get_rate() does that already.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 9d052e5a814..53cfd819779 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
ulong clk_get_parent_rate(struct clk *clk) {
const struct clk_ops *ops; struct clk *pclk;
debug("%s(clk=%p)\n", __func__, clk);
@@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk) if (IS_ERR(pclk)) return -ENODEV;
- ops = clk_dev_ops(pclk->dev);
- if (!ops->get_rate)
return -ENOSYS;- /* Read the 'rate' if not already set or if proper flag set*/ if (!pclk->rate || IS_ERR_VALUE(pclk->rate) || pclk->flags & CLK_GET_RATE_NOCACHE)
Reviewed-by: Michal Suchánek msuchanek@suse.de
Thanks
Michal
On 2/20/23 00:59, Samuel Holland wrote:
There is no need to check the parent clock's ops. The following call to clk_get_rate() does that already.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 9d052e5a814..53cfd819779 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
ulong clk_get_parent_rate(struct clk *clk) {
const struct clk_ops *ops; struct clk *pclk;
debug("%s(clk=%p)\n", __func__, clk);
@@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk) if (IS_ERR(pclk)) return -ENODEV;
- ops = clk_dev_ops(pclk->dev);
- if (!ops->get_rate)
return -ENOSYS;- /* Read the 'rate' if not already set or if proper flag set*/ if (!pclk->rate || IS_ERR_VALUE(pclk->rate) || pclk->flags & CLK_GET_RATE_NOCACHE)
Reviewed-by: Sean Anderson seanga2@gmail.com
This allows clk_get_parent() to work with non-CCF clock drivers.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/clk/clk-uclass.c | 18 ++++++++++++------ include/clk-uclass.h | 2 ++ 2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 53cfd819779..d0f8906cd60 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
struct clk *clk_get_parent(struct clk *clk) { + const struct clk_ops *ops; struct udevice *pdev; struct clk *pclk;
@@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk) if (!clk_valid(clk)) return ERR_PTR(-ENODEV);
- pdev = dev_get_parent(clk->dev); - if (!pdev) - return ERR_PTR(-ENODEV); - pclk = dev_get_clk_ptr(pdev); - if (!pclk) - return ERR_PTR(-ENODEV); + ops = clk_dev_ops(clk->dev); + if (ops->get_parent) { + pclk = ops->get_parent(clk); + } else { + pdev = dev_get_parent(clk->dev); + if (!pdev) + return ERR_PTR(-ENODEV); + pclk = dev_get_clk_ptr(pdev); + if (!pclk) + return ERR_PTR(-ENODEV); + }
return pclk; } diff --git a/include/clk-uclass.h b/include/clk-uclass.h index 65ebff9ed27..4d616720865 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -22,6 +22,7 @@ struct ofnode_phandle_args; * @round_rate: Adjust a rate to the exact rate a clock can provide. * @get_rate: Get current clock rate. * @set_rate: Set current clock rate. + * @get_parent: Get current clock parent * @set_parent: Set current clock parent * @enable: Enable a clock. * @disable: Disable a clock. @@ -36,6 +37,7 @@ struct clk_ops { ulong (*round_rate)(struct clk *clk, ulong rate); ulong (*get_rate)(struct clk *clk); ulong (*set_rate)(struct clk *clk, ulong rate); + struct clk *(*get_parent)(struct clk *clk); int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk);
On 2/20/23 00:59, Samuel Holland wrote:
This allows clk_get_parent() to work with non-CCF clock drivers.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 18 ++++++++++++------ include/clk-uclass.h | 2 ++ 2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 53cfd819779..d0f8906cd60 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
struct clk *clk_get_parent(struct clk *clk) {
- const struct clk_ops *ops; struct udevice *pdev; struct clk *pclk;
@@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk) if (!clk_valid(clk)) return ERR_PTR(-ENODEV);
- pdev = dev_get_parent(clk->dev);
- if (!pdev)
return ERR_PTR(-ENODEV);- pclk = dev_get_clk_ptr(pdev);
- if (!pclk)
return ERR_PTR(-ENODEV);
ops = clk_dev_ops(clk->dev);
if (ops->get_parent) {
pclk = ops->get_parent(clk);} else {
pdev = dev_get_parent(clk->dev);if (!pdev)return ERR_PTR(-ENODEV);pclk = dev_get_clk_ptr(pdev);if (!pclk)return ERR_PTR(-ENODEV);}
return pclk; }
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index 65ebff9ed27..4d616720865 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -22,6 +22,7 @@ struct ofnode_phandle_args;
- @round_rate: Adjust a rate to the exact rate a clock can provide.
- @get_rate: Get current clock rate.
- @set_rate: Set current clock rate.
- @get_parent: Get current clock parent
- @set_parent: Set current clock parent
- @enable: Enable a clock.
- @disable: Disable a clock.
@@ -36,6 +37,7 @@ struct clk_ops { ulong (*round_rate)(struct clk *clk, ulong rate); ulong (*get_rate)(struct clk *clk); ulong (*set_rate)(struct clk *clk, ulong rate);
- struct clk *(*get_parent)(struct clk *clk); int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk);
Missing documentation.
--Sean
Hi Samuel,
On Sun, 19 Feb 2023 at 23:00, Samuel Holland samuel@sholland.org wrote:
This allows clk_get_parent() to work with non-CCF clock drivers.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/clk/clk-uclass.c | 18 ++++++++++++------ include/clk-uclass.h | 2 ++ 2 files changed, 14 insertions(+), 6 deletions(-)
Please can you add a test for this and do full comments for the function you added?
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 53cfd819779..d0f8906cd60 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
struct clk *clk_get_parent(struct clk *clk) {
const struct clk_ops *ops; struct udevice *pdev; struct clk *pclk;@@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk) if (!clk_valid(clk)) return ERR_PTR(-ENODEV);
pdev = dev_get_parent(clk->dev);if (!pdev)return ERR_PTR(-ENODEV);pclk = dev_get_clk_ptr(pdev);if (!pclk)return ERR_PTR(-ENODEV);
ops = clk_dev_ops(clk->dev);if (ops->get_parent) {pclk = ops->get_parent(clk);} else {pdev = dev_get_parent(clk->dev);if (!pdev)return ERR_PTR(-ENODEV);pclk = dev_get_clk_ptr(pdev);if (!pclk)return ERR_PTR(-ENODEV);} return pclk;} diff --git a/include/clk-uclass.h b/include/clk-uclass.h index 65ebff9ed27..4d616720865 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -22,6 +22,7 @@ struct ofnode_phandle_args;
- @round_rate: Adjust a rate to the exact rate a clock can provide.
- @get_rate: Get current clock rate.
- @set_rate: Set current clock rate.
- @get_parent: Get current clock parent
- @set_parent: Set current clock parent
- @enable: Enable a clock.
- @disable: Disable a clock.
@@ -36,6 +37,7 @@ struct clk_ops { ulong (*round_rate)(struct clk *clk, ulong rate); ulong (*get_rate)(struct clk *clk); ulong (*set_rate)(struct clk *clk, ulong rate);
struct clk *(*get_parent)(struct clk *clk); int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk);-- 2.39.2
Regards, Simon
participants (4)
-
Michal Suchánek -
Samuel Holland -
Sean Anderson -
Simon Glass