Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 18:15, Stephen Boyd wrote:
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.
I ran the script on all files that include linux/clk.h. I've also
trimmed out mips and unicore32 because they're not using the common
clock framework.
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))
diff =
--- drivers/gpu/drm/armada/armada_510.c
+++ /tmp/cocci-output-12321-a5f298-armada_510.c
@@ -53,7 +53,6 @@ static int armada510_crtc_compute_clock(
if (IS_ERR(clk))
return PTR_ERR(clk);
- if (dcrtc-clk != clk) {
ret = clk_prepare_enable(clk);
if (ret)
return ret;
drivers/gpu/drm/armada/armada_510.c:56:5-15: WARNING trying to compare or
dereference struct clk pointers.
diff =
--- drivers/pwm/pwm-atmel-hlcdc.c
+++ /tmp/cocci-output-12679-3c5195-pwm-atmel-hlcdc.c
@@ -91,7 +91,6 @@ static int atmel_hlcdc_pwm_config(struct
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
- if (new_clk != chip-cur_clk) {
u32 gencfg = 0;
int ret;
drivers/pwm/pwm-atmel-hlcdc.c:94:5-12: WARNING trying to compare or dereference
struct clk pointers.
diff =
--- drivers/tty/serial/samsung.c
+++ /tmp/cocci-output-12827-715e72-samsung.c
@@ -750,7 +750,6 @@ static void s3c24xx_serial_set_termios(s
/* check to see if we need to change clock source */
- if (ourport-baudclk != clk) {
s3c24xx_serial_setsource(port, clk_sel);
if (!IS_ERR(ourport-baudclk)) {
drivers/tty/serial/samsung.c:753:5-21: WARNING trying to compare or dereference
struct clk pointers.
diff =
--- sound/soc/fsl/fsl_esai.c
+++ /tmp/cocci-output-13020-d518c3-fsl_esai.c
@@ -269,7 +269,6 @@ static int fsl_esai_set_dai_sysclk(struc
}
/* Only EXTAL source can be output directly without using PSR and PM */
- if (ratio == 1 clksrc == esai_priv-extalclk) {
/* Bypass all the dividers if not being needed */
ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO;
goto out;
sound/soc/fsl/fsl_esai.c:272:19-25: WARNING trying to compare or dereference
struct clk pointers.
diff =
--- sound/soc/fsl/fsl_spdif.c
+++ /tmp/cocci-output-13024-7acb1d-fsl_spdif.c
@@ -1054,7 +1054,6 @@ static u32 fsl_spdif_txclk_caldiv(struct
enum spdif_txrate index, bool round)
{
const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
- bool is_sysclk = clk == spdif_priv-sysclk;
u64 rate_ideal, rate_actual, sub;
u32 sysclk_dfmin, sysclk_dfmax;
u32 txclk_df, sysclk_df, arate;
@@ -1148,7 +1147,6 @@ static int fsl_spdif_probe_txclk(struct
spdif_priv-txclk_src[index], rate[index]);
dev_dbg(pdev-dev, use txclk df %d for %dHz sample rate\n,
spdif_priv-txclk_df[index], rate[index]);
- if (spdif_priv-txclk[index] == spdif_priv-sysclk)
dev_dbg(pdev-dev, use sysclk df %d for %dHz sample rate\n,
spdif_priv-sysclk_df[index], rate[index]);
dev_dbg(pdev-dev, the best rate for %dHz sample rate is %dHz\n,
sound/soc/fsl/fsl_spdif.c:1151:5-29: WARNING trying to compare or dereference
struct clk pointers.
sound/soc/fsl/fsl_spdif.c:1057:18-21: WARNING trying to compare or dereference
struct clk pointers.
diff =
--- sound/soc/kirkwood/kirkwood-i2s.c
+++ /tmp/cocci-output-13041-3200a6-kirkwood-i2s.c
@@ -579,7 +579,6 @@ static int kirkwood_i2s_dev_probe(struct
if (PTR_ERR(priv-extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
- if (priv-extclk == priv-clk) {
devm_clk_put(pdev-dev, priv-extclk);
priv-extclk = ERR_PTR(-EINVAL);
} else {
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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?
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__,
/// 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)
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/03/15 08:04, Quentin Lambert wrote:
Hello,
Julia asked me to have a look and see if I can help.
I have found these three cases using Coccinnelle in the mach-omap2
directory.
./arch/arm/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)
This probably needs to compare hw pointers again given that it's in the
clk-provider code. It would be nice if we could use indices instead though.
break; /* Found the requested parent */
if (!clks-parent) {
./arch/arm/mach-omap2/dpll3xxx.c
@@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
if (!dd)
return -EINVAL;
if (__clk_get_parent(hw-clk) != dd-clk_ref)
This one is what this thread has started on about. Comparing hw pointers
is ok for now...
return -EINVAL;
if (dd-last_rounded_rate == 0)
./arch/arm/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) {
This one looks like an optimization. We can just always try to set the
parent and if it's already the current parent then the core should bail
out early without an error. So the fix would be to just remove the if
condition.
r = clk_set_parent(timer-fclk, src);
if (r 0) {
pr_warn(%s: %s cannot set source\n, __func__,
Are they instances of your issue?
Yes these all look like instances of the problem.
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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Hello,
Julia asked me to have a look and see if I can help.
On 02/02/2015 23:50, Mike Turquette wrote:
Quoting Stephen Boyd (2015-02-02 14:35:59)
On 02/02/15 13:31, Julia Lawall wrote:
On Mon, 2 Feb 2015, Stephen Boyd wrote:
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
Comparing or dereferencing pointers of a particular type should be
straightforward to check for. Is there an example of how to use the
parent_index value to fix the problem?
I'm not sure how to fix this case with parent_index values generically.
I imagine it would be highly specific to the surrounding code to the
point where we couldn't fix it automatically. For example, if it's a clk
consumer (not provider like in this case) using an index wouldn't be the
right fix. We would need some sort of clk API that we don't currently
have and I would wonder why clock consumers even care to compare such
pointers in the first place.
Ack. Is there precedent for a Don't do that kind of response?
Regards,
Mike
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Cocci mailing list
co...@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
I have found these three cases using Coccinnelle in the mach-omap2
directory.
./arch/arm/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) {
./arch/arm/mach-omap2/dpll3xxx.c
@@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
if (!dd)
return -EINVAL;
if (__clk_get_parent(hw-clk) != dd-clk_ref)
return -EINVAL;
if (dd-last_rounded_rate == 0)
./arch/arm/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__,
Are they instances of your issue?
Do you have any suggestion on how to fix them?
If you want me to I can enlarge the search to other directories.
Quentin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
