| Message ID | 20240725073707.26040-1-Zhiqiang.Hou@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | clk: fix ccf_clk_get_rate | expand |
Hi On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote: > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > As the type of return value is 'ulong', when clk_get_by_id() failed, > it should return 0 to indicate the get_rate operation doesn't succeed. > I understand your point here but the clk_get_rate that can call the ccf clk_get_rate can already return -ENOSYS. Michael > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > --- > drivers/clk/clk.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index b8c2e8d531..4c2c372cd3 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -84,7 +84,8 @@ ulong ccf_clk_get_rate(struct clk *clk) > int err = clk_get_by_id(clk->id, &c); > > if (err) > - return err; > + return 0; > + > return clk_get_rate(c); > } > > -- > 2.17.1 >
Hi Michael, > -----Original Message----- > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > Sent: Thursday, July 25, 2024 3:32 PM > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > seanga2@gmail.com > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > Hi > > On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> > wrote: > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > As the type of return value is 'ulong', when clk_get_by_id() failed, > > it should return 0 to indicate the get_rate operation doesn't succeed. > > > > I understand your point here but the clk_get_rate that can call the ccf > clk_get_rate can already return -ENOSYS. will also fix it. Thanks, Zhiqiang
Hi On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote: > > Hi Michael, > > > -----Original Message----- > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > Sent: Thursday, July 25, 2024 3:32 PM > > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > seanga2@gmail.com > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > Hi > > > > On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> > > wrote: > > > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > As the type of return value is 'ulong', when clk_get_by_id() failed, > > > it should return 0 to indicate the get_rate operation doesn't succeed. > > > > > > > I understand your point here but the clk_get_rate that can call the ccf > > clk_get_rate can already return -ENOSYS. > > will also fix it. > Is there any usage of the error set on the latest bit of the clock? We need to be sure that this is correct use accross the uboot. The clk-uclass define the error that can return Michael > Thanks, > Zhiqiang -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com
Hi Michael, > -----Original Message----- > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > Sent: Thursday, July 25, 2024 5:26 PM > To: Z.Q. Hou <zhiqiang.hou@nxp.com>; Simon Glass <sjg@chromium.org> > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > seanga2@gmail.com > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > Hi > > On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote: > > > > Hi Michael, > > > > > -----Original Message----- > > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > > Sent: Thursday, July 25, 2024 3:32 PM > > > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > > seanga2@gmail.com > > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > > > Hi > > > > > > On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> > > > wrote: > > > > > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > > > As the type of return value is 'ulong', when clk_get_by_id() > > > > failed, it should return 0 to indicate the get_rate operation doesn't > succeed. > > > > > > > > > > I understand your point here but the clk_get_rate that can call the > > > ccf clk_get_rate can already return -ENOSYS. > > > > will also fix it. > > > > Is there any usage of the error set on the latest bit of the clock? We need to > be sure that this is correct use accross the uboot. The clk-uclass define the > error that can return The problem is this function's return value is 'ulong', if use the error set, the caller will treat it as a good value instead of a error number. There are several APIs have the same problem. 2 methods to fix it: 1. keep the API's prototype, and use '0' to indicate error condition, but a real '0' return value is also considered as error. 2. Change the return type to like 'long long int', and use a negative error number to indicate error condition. Need to update the check of return value for all the callers. Any suggestion? Thanks, Zhiqiang
Hi On Thu, Jul 25, 2024 at 11:58 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote: > > Hi Michael, > > > -----Original Message----- > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > Sent: Thursday, July 25, 2024 5:26 PM > > To: Z.Q. Hou <zhiqiang.hou@nxp.com>; Simon Glass <sjg@chromium.org> > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > seanga2@gmail.com > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > Hi > > > > On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote: > > > > > > Hi Michael, > > > > > > > -----Original Message----- > > > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > > > Sent: Thursday, July 25, 2024 3:32 PM > > > > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > > > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > > > seanga2@gmail.com > > > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > > > > > Hi > > > > > > > > On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> > > > > wrote: > > > > > > > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > > > > > As the type of return value is 'ulong', when clk_get_by_id() > > > > > failed, it should return 0 to indicate the get_rate operation doesn't > > succeed. > > > > > > > > > > > > > I understand your point here but the clk_get_rate that can call the > > > > ccf clk_get_rate can already return -ENOSYS. > > > > > > will also fix it. > > > > > > > Is there any usage of the error set on the latest bit of the clock? We need to > > be sure that this is correct use accross the uboot. The clk-uclass define the > > error that can return > > The problem is this function's return value is 'ulong', if use the error set, the caller will treat it as a good value instead of a error number. There are several APIs have the same problem. > > 2 methods to fix it: > 1. keep the API's prototype, and use '0' to indicate error condition, but a real '0' return value is also considered as error. > 2. Change the return type to like 'long long int', and use a negative error number to indicate error condition. Need to update the check of return value for all the callers. > > Any suggestion? > unit test suggest that error condition are evaluated so we have 32 bit on arm of unsigned long so we can not map all the positive clock rate = clk_set_rate(clk, 80000000); ut_asserteq(rate, -ENOSYS); Now, can we have a clock greater then 2Ghz if yes this not work always on 32 bit, then the macro IS_ERR_VALUE should help on clk return Michael > Thanks, > Zhiqiang -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com
Hi Michael, > -----Original Message----- > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > Sent: Thursday, July 25, 2024 6:21 PM > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > Cc: Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de; > trini@konsulko.com; lukma@denx.de; seanga2@gmail.com > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > Hi > > On Thu, Jul 25, 2024 at 11:58 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote: > > > > Hi Michael, > > > > > -----Original Message----- > > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > > Sent: Thursday, July 25, 2024 5:26 PM > > > To: Z.Q. Hou <zhiqiang.hou@nxp.com>; Simon Glass <sjg@chromium.org> > > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > > seanga2@gmail.com > > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > > > Hi > > > > > > On Thu, Jul 25, 2024 at 10:53 AM Z.Q. Hou <zhiqiang.hou@nxp.com> > wrote: > > > > > > > > Hi Michael, > > > > > > > > > -----Original Message----- > > > > > From: Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> > > > > > Sent: Thursday, July 25, 2024 3:32 PM > > > > > To: Z.Q. Hou <zhiqiang.hou@nxp.com> > > > > > Cc: u-boot@lists.denx.de; trini@konsulko.com; lukma@denx.de; > > > > > seanga2@gmail.com > > > > > Subject: Re: [PATCH] clk: fix ccf_clk_get_rate > > > > > > > > > > Hi > > > > > > > > > > On Thu, Jul 25, 2024 at 9:16 AM Zhiqiang Hou > > > > > <Zhiqiang.Hou@nxp.com> > > > > > wrote: > > > > > > > > > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > > > > > > > As the type of return value is 'ulong', when clk_get_by_id() > > > > > > failed, it should return 0 to indicate the get_rate operation > > > > > > doesn't > > > succeed. > > > > > > > > > > > > > > > > I understand your point here but the clk_get_rate that can call > > > > > the ccf clk_get_rate can already return -ENOSYS. > > > > > > > > will also fix it. > > > > > > > > > > Is there any usage of the error set on the latest bit of the clock? > > > We need to be sure that this is correct use accross the uboot. The > > > clk-uclass define the error that can return > > > > The problem is this function's return value is 'ulong', if use the error set, the > caller will treat it as a good value instead of a error number. There are several > APIs have the same problem. > > > > 2 methods to fix it: > > 1. keep the API's prototype, and use '0' to indicate error condition, but a real > '0' return value is also considered as error. > > 2. Change the return type to like 'long long int', and use a negative error > number to indicate error condition. Need to update the check of return value > for all the callers. > > > > Any suggestion? > > > > unit test suggest that error condition are evaluated so we have 32 bit on arm > of unsigned long so we can not map all the positive clock > > rate = clk_set_rate(clk, 80000000); > ut_asserteq(rate, -ENOSYS); > > Now, can we have a clock greater then 2Ghz if yes this not work always on 32 > bit, then the macro IS_ERR_VALUE should help on clk return You mean keep it as it's and let the callers evaluate the error condition using IS_ERR_VALUE, right? Current implement also make 'return 0' as error condition, and many callers treat all non-zero return value as good, these need to fix. ulong clk_get_rate(struct clk *clk) { ... if (!clk_valid(clk)) return 0; ... } Contrast to reserve 4K errnos, I prefer to 'return 0' as error condition, 0 Hz is almost never a correct value, and all the positive value can be used especially on 32-bit platform, does the caller really cares about the exact error number? Thanks, Zhiqiang
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b8c2e8d531..4c2c372cd3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -84,7 +84,8 @@ ulong ccf_clk_get_rate(struct clk *clk) int err = clk_get_by_id(clk->id, &c); if (err) - return err; + return 0; + return clk_get_rate(c); }