Re: [U-Boot] Question about compile warnings of exynos clock
Hi,
Hi,
On 01/13/2015 11:56 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:51, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:47 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:36, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:27 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:24, Joonyoung Shim jy0922.shim@samsung.com wrote: > Hi, > > I found below compile warnings, > > CC arch/arm/cpu/armv7/exynos/clock.o > arch/arm/cpu/armv7/exynos/clock.c: In function .clock_get_periph_rate.: > arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] > struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; > ^ > arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] > > ... >> static unsigned long exynos5_get_periph_rate(int peripheral) >> { >> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >> > > This can access out of bounds of clk_bit_info[] array from > exynos5_get_periph_rate(). The peripheral value comes from > enum periph_id but it gets out of count clk_bit_info[] array. > > So, i don't think exynos5_get_periph_rate is working correctly. > Currently, exynos5_get_periph_rate is used by clock_get_periph_rate only > from get_pwm_clk. > > Is it ongoing to work for generic api to get the clk freq? If not, > let's remove exynos5_get_periph_rate and clock_get_periph_rate.
That's going in the wrong direction - these functions make the code much easier to follow and refactor. We should remove get_pwm_clk(), get_mmc_clk() etc. and use generic functions instead.
I know, but current codes are wrong, so first i want to correct it even if it is old way because it's really easy. And then we can go to generic functions.
So not remove the generic functions?
Is it this line which is broken?
clock_get_periph_rate(PERIPH_ID_PWM0);
If so, it seems like everything else uses its own function. It should all move easily to the generic function I think.
The basic problem is the generic function is wrong.
Oh dear, does it not work for anything?
I have posted the patch relevant to this. I removed the generic peripheral clock API. It's not used anywhere. Just get clock for pwm0.
how about?
I dont think it's a good idea to remove this function and expect to put in same effort again in future. Instead, fixing this warning should be easier.
By the way, I don't see the mentioned warning when I compile ToT u-boot-samsung for smdk5250 and smdk5420 with arm-2011.09 compiler. Do we need any other patch or config to encounter this?
Best Regards, Jaehoon Chung
Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Akshay Saraswat
Hi Akshay,
On 01/13/2015 02:40 PM, Akshay Saraswat wrote:
Hi,
Hi,
On 01/13/2015 11:56 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:51, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:47 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:36, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:27 AM, Simon Glass wrote: > Hi, > > On 12 January 2015 at 18:24, Joonyoung Shim jy0922.shim@samsung.com wrote: >> Hi, >> >> I found below compile warnings, >> >> CC arch/arm/cpu/armv7/exynos/clock.o >> arch/arm/cpu/armv7/exynos/clock.c: In function .clock_get_periph_rate.: >> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >> ^ >> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >> >> ... >>> static unsigned long exynos5_get_periph_rate(int peripheral) >>> { >>> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >>> >> >> This can access out of bounds of clk_bit_info[] array from >> exynos5_get_periph_rate(). The peripheral value comes from >> enum periph_id but it gets out of count clk_bit_info[] array. >> >> So, i don't think exynos5_get_periph_rate is working correctly. >> Currently, exynos5_get_periph_rate is used by clock_get_periph_rate only >> from get_pwm_clk. >> >> Is it ongoing to work for generic api to get the clk freq? If not, >> let's remove exynos5_get_periph_rate and clock_get_periph_rate. > > That's going in the wrong direction - these functions make the code > much easier to follow and refactor. We should remove get_pwm_clk(), > get_mmc_clk() etc. and use generic functions instead. >
I know, but current codes are wrong, so first i want to correct it even if it is old way because it's really easy. And then we can go to generic functions.
So not remove the generic functions?
Is it this line which is broken?
clock_get_periph_rate(PERIPH_ID_PWM0);
If so, it seems like everything else uses its own function. It should all move easily to the generic function I think.
The basic problem is the generic function is wrong.
Oh dear, does it not work for anything?
I have posted the patch relevant to this. I removed the generic peripheral clock API. It's not used anywhere. Just get clock for pwm0.
how about?
I dont think it's a good idea to remove this function and expect to put in same effort again in future. Instead, fixing this warning should be easier.
Then, do you have any plan to fix it and convert to use generic clock get api?
By the way, I don't see the mentioned warning when I compile ToT u-boot-samsung for smdk5250 and smdk5420 with arm-2011.09 compiler. Do we need any other patch or config to encounter this?
I used toolchain of linaro
http://releases.linaro.org/14.09/components/toolchain/binaries/gcc-linaro-ar...
Thanks.
Hi.
On 01/13/2015 02:40 PM, Akshay Saraswat wrote:
Hi,
Hi,
On 01/13/2015 11:56 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:51, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:47 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:36, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:27 AM, Simon Glass wrote: > Hi, > > On 12 January 2015 at 18:24, Joonyoung Shim jy0922.shim@samsung.com wrote: >> Hi, >> >> I found below compile warnings, >> >> CC arch/arm/cpu/armv7/exynos/clock.o >> arch/arm/cpu/armv7/exynos/clock.c: In function .clock_get_periph_rate.: >> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >> ^ >> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >> >> ... >>> static unsigned long exynos5_get_periph_rate(int peripheral) >>> { >>> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >>> >> >> This can access out of bounds of clk_bit_info[] array from >> exynos5_get_periph_rate(). The peripheral value comes from >> enum periph_id but it gets out of count clk_bit_info[] array. >> >> So, i don't think exynos5_get_periph_rate is working correctly. >> Currently, exynos5_get_periph_rate is used by clock_get_periph_rate only >> from get_pwm_clk. >> >> Is it ongoing to work for generic api to get the clk freq? If not, >> let's remove exynos5_get_periph_rate and clock_get_periph_rate. > > That's going in the wrong direction - these functions make the code > much easier to follow and refactor. We should remove get_pwm_clk(), > get_mmc_clk() etc. and use generic functions instead. >
I know, but current codes are wrong, so first i want to correct it even if it is old way because it's really easy. And then we can go to generic functions.
So not remove the generic functions?
Is it this line which is broken?
clock_get_periph_rate(PERIPH_ID_PWM0);
If so, it seems like everything else uses its own function. It should all move easily to the generic function I think.
The basic problem is the generic function is wrong.
Oh dear, does it not work for anything?
I have posted the patch relevant to this. I removed the generic peripheral clock API. It's not used anywhere. Just get clock for pwm0.
how about?
I dont think it's a good idea to remove this function and expect to put in same effort again in future. Instead, fixing this warning should be easier.
By the way, I don't see the mentioned warning when I compile ToT u-boot-samsung for smdk5250 and smdk5420 with arm-2011.09 compiler. Do we need any other patch or config to encounter this?
It is critical problem. I don't understand how it's working fine. PERIPH_ID_PWM0 is 132, that's out of boundary for clk_bit_info. isn't? Then what's clk_bit_info[132]'s value and how access it? If you fix this problem, we need not to remove clock_get_periph_rate().. But if we need to wait for long time, i want to remove this api.
Best Regards, Jaehoon Chung
Best Regards, Jaehoon Chung
Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Akshay Saraswat
Hi,
On 12 January 2015 at 21:58, Jaehoon Chung jh80.chung@samsung.com wrote:
Hi.
On 01/13/2015 02:40 PM, Akshay Saraswat wrote:
Hi,
Hi,
On 01/13/2015 11:56 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:51, Joonyoung Shim jy0922.shim@samsung.com wrote:
Hi,
On 01/13/2015 11:47 AM, Simon Glass wrote:
Hi,
On 12 January 2015 at 18:36, Joonyoung Shim jy0922.shim@samsung.com wrote: > Hi, > > On 01/13/2015 11:27 AM, Simon Glass wrote: >> Hi, >> >> On 12 January 2015 at 18:24, Joonyoung Shim jy0922.shim@samsung.com wrote: >>> Hi, >>> >>> I found below compile warnings, >>> >>> CC arch/arm/cpu/armv7/exynos/clock.o >>> arch/arm/cpu/armv7/exynos/clock.c: In function .clock_get_periph_rate.: >>> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >>> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >>> ^ >>> arch/arm/cpu/armv7/exynos/clock.c:265:47: warning: array subscript is above array bounds [-Warray-bounds] >>> >>> ... >>>> static unsigned long exynos5_get_periph_rate(int peripheral) >>>> { >>>> struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; >>>> >>> >>> This can access out of bounds of clk_bit_info[] array from >>> exynos5_get_periph_rate(). The peripheral value comes from >>> enum periph_id but it gets out of count clk_bit_info[] array. >>> >>> So, i don't think exynos5_get_periph_rate is working correctly. >>> Currently, exynos5_get_periph_rate is used by clock_get_periph_rate only >>> from get_pwm_clk. >>> >>> Is it ongoing to work for generic api to get the clk freq? If not, >>> let's remove exynos5_get_periph_rate and clock_get_periph_rate. >> >> That's going in the wrong direction - these functions make the code >> much easier to follow and refactor. We should remove get_pwm_clk(), >> get_mmc_clk() etc. and use generic functions instead. >> > > I know, but current codes are wrong, so first i want to correct it even > if it is old way because it's really easy. And then we can go to generic > functions.
So not remove the generic functions?
Is it this line which is broken?
clock_get_periph_rate(PERIPH_ID_PWM0);
If so, it seems like everything else uses its own function. It should all move easily to the generic function I think.
The basic problem is the generic function is wrong.
Oh dear, does it not work for anything?
I have posted the patch relevant to this. I removed the generic peripheral clock API. It's not used anywhere. Just get clock for pwm0.
how about?
I dont think it's a good idea to remove this function and expect to put in same effort again in future. Instead, fixing this warning should be easier.
By the way, I don't see the mentioned warning when I compile ToT u-boot-samsung for smdk5250 and smdk5420 with arm-2011.09 compiler. Do we need any other patch or config to encounter this?
It is critical problem. I don't understand how it's working fine. PERIPH_ID_PWM0 is 132, that's out of boundary for clk_bit_info. isn't? Then what's clk_bit_info[132]'s value and how access it? If you fix this problem, we need not to remove clock_get_periph_rate().. But if we need to wait for long time, i want to remove this api.
Best Regards, Jaehoon Chung
It looks like Akshay has sent a patch super-fast Does that resolve the issue?
Regards, Simon
participants (4)
-
Akshay Saraswat -
Jaehoon Chung -
Joonyoung Shim -
Simon Glass