diff mbox

[Cocci,v13,3/6] clk: Make clk API return per-user struct clk instances

Message ID 54D3900A.9060200@gmail.com
State New
Headers show

Commit Message

Quentin Lambert Feb. 5, 2015, 3:45 p.m. UTC 
On 05/02/2015 00:26, Stephen Boyd wrote:
>> If you want me to I can enlarge the search to other directories.
> Yes please do. And if you could share the coccinelle patch that would be
> great. Thanks.
>
structclk.cocci is the coccinelle patch
structclk-arm.patch is the result I got when applying it to the arch/arm 
directory

Is there anything else I can do to help?
/// Find any attempt to compare or dereference struct clk pointers.
///
// Confidence: High
// Copyright: (C) 2015 Quentin Lambert, INRIA/LiP6. GPLv2
// URL: http://coccinelle.lip6.fr/
// Options: --recursive-includes --relax-include-path
// Options: --include-headers-for-types

virtual context
virtual org
virtual report


// ----------------------------------------------------------------------------

@comparison_dereference depends on context || org || report@
struct clk *x1, x2;
position j0;
@@

(
*  x1@j0 == x2
|
*  x1@j0 != x2
|
*  *x1@j0
)

// ----------------------------------------------------------------------------

@script:python comparison_dereference_org depends on org@
j0 << comparison_dereference.j0;
@@

msg = "WARNING trying to compare or dereference struct clk pointers."
coccilib.org.print_todo(j0[0], msg)

// ----------------------------------------------------------------------------

@script:python comparison_dereference_report depends on report@
j0 << comparison_dereference.j0;
@@

msg = "WARNING trying to compare or dereference struct clk pointers."
coccilib.report.print_report(j0[0], msg)

Comments

Quentin Lambert Feb. 5, 2015, 4:02 p.m. UTC | #1 
Sorry let me do that properly.

On 05/02/2015 16:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the 
> arch/arm directory
>
> Is there anything else I can do to help?
>
>

These are the new instances I found by applying the patch to arch/arm 
directory:

./arch/arm/mach-imx/mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
       * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
       * (external OSC), and we need to clear the bit.
       */
      clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
                         IMX6Q_GPR1_ENET_CLK_SEL_PAD;
      gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
      if (!IS_ERR(gpr))

./arch/arm/mach-shmobile/clock-r8a73a4.c
@@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)

./arch/arm/mach-shmobile/clock-sh7372.c
@@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)

./arch/arm/mach-shmobile/clock-r8a7740.c
@@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)
Stephen Boyd Feb. 6, 2015, 1:49 a.m. UTC | #2 
On 02/05/15 08:02, Quentin Lambert wrote:
> Sorry let me do that properly.
>
> On 05/02/2015 16:45, Quentin Lambert wrote:
>>
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that
>>> would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
>
> These are the new instances I found by applying the patch to arch/arm
> directory:
>
> ./arch/arm/mach-imx/mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>       * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>       * (external OSC), and we need to clear the bit.
>       */
>      clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>                         IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>      gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>      if (!IS_ERR(gpr))

This one looks like a real problem and it could probably use a
clk_equal(struct clk *a, struct clk *b) function.

>
> ./arch/arm/mach-shmobile/clock-r8a73a4.c
> @@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-sh7372.c
> @@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-r8a7740.c
> @@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
>
>

I don't think shmobile uses the CCF so this should be safe, but we
should probably fix them up to also use a clk_equal() function that just
does pointer comparisons.
Stephen Boyd Feb. 6, 2015, 2:15 a.m. UTC | #3 
On 02/05/15 07:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the
> arch/arm directory
>
> Is there anything else I can do to help?
>
>

Thanks for the coccinelle patch. Thinking more about it, I don't think
we care if the pointer is dereferenced because that would require a
definition of struct clk and that is most likely not the case outside of
the clock framework. Did you scan the entire kernel? I'm running it now
but it seems to be taking a while.
Quentin Lambert Feb. 6, 2015, 9:01 a.m. UTC | #4 
On 06/02/2015 03:15, Stephen Boyd wrote:
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel?
No I haven't.
> I'm running it now
> but it seems to be taking a while.
>
Yes, that's why, as a first step, I chose to limit the scan to the arm 
directory.
Julia Lawall Feb. 6, 2015, 9:12 a.m. UTC | #5 
On Fri, 6 Feb 2015, Quentin Lambert wrote:

>
> On 06/02/2015 03:15, Stephen Boyd wrote:
> > Thanks for the coccinelle patch. Thinking more about it, I don't think
> > we care if the pointer is dereferenced because that would require a
> > definition of struct clk and that is most likely not the case outside of
> > the clock framework. Did you scan the entire kernel?
> No I haven't.
> > I'm running it now
> > but it seems to be taking a while.
> >
> Yes, that's why, as a first step, I chose to limit the scan to the arm
> directory.

Are you sure to be using all of the options provided:

// Options: --recursive-includes --relax-include-path
// Options: --include-headers-for-types

And are you using 1.0.0-rc23 or 1.0.0-rc24?  Those should save parsed
header files so that they don't have to be parsed over and over.

If you are using rc24, then you can use the -j option for parallelism.
But then you should also use an option like --chunksize 10 (I don't know
what number would be good), because the default is chunksize 1, and in
that case the saved parsed header files are not reused, because the fies
are all processed individually.  In general, it is only the files within a
chunk that will share parsed header files.

julia
Stephen Boyd Feb. 6, 2015, 5:15 p.m. UTC | #6 
On 02/06/15 01:12, Julia Lawall wrote:
>
> On Fri, 6 Feb 2015, Quentin Lambert wrote:
>
>> On 06/02/2015 03:15, Stephen Boyd wrote:
>>> Thanks for the coccinelle patch. Thinking more about it, I don't think
>>> we care if the pointer is dereferenced because that would require a
>>> definition of struct clk and that is most likely not the case outside of
>>> the clock framework. Did you scan the entire kernel?
>> No I haven't.
>>> I'm running it now
>>> but it seems to be taking a while.
>>>
>> Yes, that's why, as a first step, I chose to limit the scan to the arm
>> directory.
> Are you sure to be using all of the options provided:
>
> // Options: --recursive-includes --relax-include-path
> // Options: --include-headers-for-types
>
> And are you using 1.0.0-rc23 or 1.0.0-rc24?  Those should save parsed
> header files so that they don't have to be parsed over and over.
>
> If you are using rc24, then you can use the -j option for parallelism.
> But then you should also use an option like --chunksize 10 (I don't know
> what number would be good), because the default is chunksize 1, and in
> that case the saved parsed header files are not reused, because the fies
> are all processed individually.  In general, it is only the files within a
> chunk that will share parsed header files.

Thanks for the info.

$ spatch --version
spatch version 1.0.0-rc22 with Python support and with PCRE support

so I guess I need to update. I tried throwing it into
scripts/coccinelle/misc and then did

make O=../obj/ coccicheck
COCCI=../kernel/scripts/coccinelle/misc/structclk.cocci

at the toplevel but it didn't find anything.
Sebastian Andrzej Siewior March 12, 2015, 5:20 p.m. UTC | #7 
On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> diff = 
> --- arch/arm/mach-imx/mach-imx6q.c
> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>  	 * (external OSC), and we need to clear the bit.
>  	 */
> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>  	if (!IS_ERR(gpr))

Any idea how to do the comparison here? Or should we rely that the bootloader
sets this properly (it managed already to select a frequency)? The phy has no
clock node in current DT's so we can check this.

Sebastian
Stephen Boyd March 12, 2015, 7:43 p.m. UTC | #8 
On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>> diff = 
>> --- arch/arm/mach-imx/mach-imx6q.c
>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>  	 * (external OSC), and we need to clear the bit.
>>  	 */
>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>  	if (!IS_ERR(gpr))
> Any idea how to do the comparison here? Or should we rely that the bootloader
> sets this properly (it managed already to select a frequency)? The phy has no
> clock node in current DT's so we can check this.
>

This has been fixed by adding a clk_is_match() helper and using that to
compare instead of comparing raw pointers. It would be nice if we could
replace the patch with something else that doesn't require this helper
though. It looks like this is static board configuration, so I wonder
why we didn't just have a DT property that indicates how the gpr should
be configured for this particular board.
Shawn Guo March 13, 2015, 3:29 a.m. UTC | #9 
On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> > On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> >> diff = 
> >> --- arch/arm/mach-imx/mach-imx6q.c
> >> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> >> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
> >>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
> >>  	 * (external OSC), and we need to clear the bit.
> >>  	 */
> >> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
> >>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
> >>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> >>  	if (!IS_ERR(gpr))
> > Any idea how to do the comparison here? Or should we rely that the bootloader
> > sets this properly (it managed already to select a frequency)? The phy has no
> > clock node in current DT's so we can check this.
> >
> 
> This has been fixed by adding a clk_is_match() helper and using that to
> compare instead of comparing raw pointers. It would be nice if we could
> replace the patch with something else that doesn't require this helper
> though. It looks like this is static board configuration, so I wonder
> why we didn't just have a DT property that indicates how the gpr should
> be configured for this particular board.

We did not add a DT property for it, because there was already enough
info (clock configuration) in DT for kernel to figure it out.

Shawn
Sebastian Andrzej Siewior March 13, 2015, 8:20 a.m. UTC | #10 
Hi Shawn,

On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:

> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.
Correct. My understanding is whatever can be figured out without DT should
be done that way.

Is there a way to get this clock-select bit set without
enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
frequency and also sets the proper bit in gpr1. My question is simply why do
we need to do this here as well.

> Shawn

Sebastian
Shawn Guo March 13, 2015, 1:42 p.m. UTC | #11 
On Fri, Mar 13, 2015 at 09:20:10AM +0100, Sebastian Andrzej Siewior wrote:
> Hi Shawn,
> 
> On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:
> 
> > We did not add a DT property for it, because there was already enough
> > info (clock configuration) in DT for kernel to figure it out.
> Correct. My understanding is whatever can be figured out without DT should
> be done that way.
> 
> Is there a way to get this clock-select bit set without
> enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
> frequency and also sets the proper bit in gpr1. My question is simply why do
> we need to do this here as well.

I'm not sure I follow your question.  But we had been doing this setup
in kernel function imx6q_1588_init() for a while.  The problem we're
having now is that the clk core change broke it since v4.0-rc1.  And the
fix is already queued there [1].

Shawn

[1] https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-fixes
Stephen Boyd March 13, 2015, 5:42 p.m. UTC | #12 
On 03/12/15 20:29, Shawn Guo wrote:
> On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
>> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
>>> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>>>> diff = 
>>>> --- arch/arm/mach-imx/mach-imx6q.c
>>>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>>>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>>>  	 * (external OSC), and we need to clear the bit.
>>>>  	 */
>>>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>>>  	if (!IS_ERR(gpr))
>>> Any idea how to do the comparison here? Or should we rely that the bootloader
>>> sets this properly (it managed already to select a frequency)? The phy has no
>>> clock node in current DT's so we can check this.
>>>
>> This has been fixed by adding a clk_is_match() helper and using that to
>> compare instead of comparing raw pointers. It would be nice if we could
>> replace the patch with something else that doesn't require this helper
>> though. It looks like this is static board configuration, so I wonder
>> why we didn't just have a DT property that indicates how the gpr should
>> be configured for this particular board.
> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.

Ok well then if everything is in DT we don't need to be comparing clock
pointers at all, right? We should be able to write up some DT parsing
logic to figure it out?
diff mbox

Patch

diff -u -p ./arch/arm/mach-imx/mach-imx6q.c /tmp/nothing/mach-imx/mach-imx6q.c
--- ./arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/nothing/mach-imx/mach-imx6q.c
@@ -211,7 +211,6 @@  static void __init imx6q_1588_init(void)
 	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
 	 * (external OSC), and we need to clear the bit.
 	 */
-	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
 	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (!IS_ERR(gpr))
diff -u -p ./arch/arm/mach-shmobile/clock-r8a73a4.c /tmp/nothing/mach-shmobile/clock-r8a73a4.c
--- ./arch/arm/mach-shmobile/clock-r8a73a4.c
+++ /tmp/nothing/mach-shmobile/clock-r8a73a4.c
@@ -139,7 +139,6 @@  static int pll_set_parent(struct clk *cl
 
 	/* Search the parent */
 	for (i = 0; i < clk->parent_num; i++)
-		if (clk->parent_table[i] == parent)
 			break;
 
 	if (i == clk->parent_num)
diff -u -p ./arch/arm/mach-shmobile/clock-sh7372.c /tmp/nothing/mach-shmobile/clock-sh7372.c
--- ./arch/arm/mach-shmobile/clock-sh7372.c
+++ /tmp/nothing/mach-shmobile/clock-sh7372.c
@@ -223,7 +223,6 @@  static int pllc2_set_parent(struct clk *
 
 	/* Search the parent */
 	for (i = 0; i < clk->parent_num; i++)
-		if (clk->parent_table[i] == parent)
 			break;
 
 	if (i == clk->parent_num)
diff -u -p ./arch/arm/mach-shmobile/clock-r8a7740.c /tmp/nothing/mach-shmobile/clock-r8a7740.c
--- ./arch/arm/mach-shmobile/clock-r8a7740.c
+++ /tmp/nothing/mach-shmobile/clock-r8a7740.c
@@ -195,7 +195,6 @@  static int usb24s_set_parent(struct clk
 
 	/* Search the parent */
 	for (i = 0; i < clk->parent_num; i++)
-		if (clk->parent_table[i] == parent)
 			break;
 
 	if (i == clk->parent_num)
diff -u -p ./arch/arm/mach-omap2/clkt_clksel.c /tmp/nothing/mach-omap2/clkt_clksel.c
--- ./arch/arm/mach-omap2/clkt_clksel.c
+++ /tmp/nothing/mach-omap2/clkt_clksel.c
@@ -67,7 +67,6 @@  static const struct clksel *_get_clksel_
 		return NULL;
 
 	for (clks = clk->clksel; clks->parent; clks++)
-		if (clks->parent == src_clk)
 			break; /* Found the requested parent */
 
 	if (!clks->parent) {
diff -u -p ./arch/arm/mach-omap2/timer.c /tmp/nothing/mach-omap2/timer.c
--- ./arch/arm/mach-omap2/timer.c
+++ /tmp/nothing/mach-omap2/timer.c
@@ -298,7 +298,6 @@  static int __init omap_dm_timer_init_one
 	if (IS_ERR(src))
 		return PTR_ERR(src);
 
-	if (clk_get_parent(timer->fclk) != src) {
 		r = clk_set_parent(timer->fclk, src);
 		if (r < 0) {
 			pr_warn("%s: %s cannot set source\n", __func__,