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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 19, 2015 at 01:32:33PM -0800, Mike Turquette wrote: Quoting Stephen Boyd (2015-02-06 11:30:18) On 02/06/15 05:39, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this return unique clocks. Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then extclk in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then external for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning no external clock specified. Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd sb...@codeaurora.org Russell et al, I'm happy to take this patch through the clock tree (where the problem shows up) with an ack. It's not up to me - I don't maintain this driver. I'm just an interested party. Note that much more than just this has now broken. The iMX6 code has broken as well, and it's not going to take such a simple fix there to fix it either. Please either revert the patches creating this breakage (and have another attempt at the next merge window) or supply fixes for these places. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-06 11:30:18) On 02/06/15 05:39, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this return unique clocks. Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then extclk in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then external for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning no external clock specified. Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd sb...@codeaurora.org Russell et al, I'm happy to take this patch through the clock tree (where the problem shows up) with an ack. Regards, Mike --- arch/arm/mach-dove/common.c | 4 ++-- sound/soc/kirkwood/kirkwood-i2s.c | 13 - 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index 0d1a89298ece..f290fc944cc1 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -124,8 +124,8 @@ static void __init dove_clk_init(void) orion_clkdev_add(NULL, sdhci-dove.1, sdio1); orion_clkdev_add(NULL, orion_nand, nand); orion_clkdev_add(NULL, cafe1000-ccic.0, camera); - orion_clkdev_add(NULL, mvebu-audio.0, i2s0); - orion_clkdev_add(NULL, mvebu-audio.1, i2s1); + orion_clkdev_add(internal, mvebu-audio.0, i2s0); + orion_clkdev_add(internal, mvebu-audio.1, i2s1); orion_clkdev_add(NULL, mv_crypto, crypto); orion_clkdev_add(NULL, dove-ac97, ac97); orion_clkdev_add(NULL, dove-pdma, pdma); diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index def7d8260c4e..0bfeb712a997 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -EINVAL; } - priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL); + priv-clk = devm_clk_get(pdev-dev, internal); if (IS_ERR(priv-clk)) { dev_err(pdev-dev, no clock\n); return PTR_ERR(priv-clk); @@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) if
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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. We _could_, and that would solve this specific issue, but I'd suggest coccinelle is used to locate any other similar instances of this. As I'm allergic to coccinelle (or coccinelle is allergic to me since I never seem to be able to get it to do what I want...) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 05:39, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this return unique clocks. Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then extclk in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then external for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning no external clock specified. Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd sb...@codeaurora.org --- arch/arm/mach-dove/common.c | 4 ++-- sound/soc/kirkwood/kirkwood-i2s.c | 13 - 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index 0d1a89298ece..f290fc944cc1 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -124,8 +124,8 @@ static void __init dove_clk_init(void) orion_clkdev_add(NULL, sdhci-dove.1, sdio1); orion_clkdev_add(NULL, orion_nand, nand); orion_clkdev_add(NULL, cafe1000-ccic.0, camera); - orion_clkdev_add(NULL, mvebu-audio.0, i2s0); - orion_clkdev_add(NULL, mvebu-audio.1, i2s1); + orion_clkdev_add(internal, mvebu-audio.0, i2s0); + orion_clkdev_add(internal, mvebu-audio.1, i2s1); orion_clkdev_add(NULL, mv_crypto, crypto); orion_clkdev_add(NULL, dove-ac97, ac97); orion_clkdev_add(NULL, dove-pdma, pdma); diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index def7d8260c4e..0bfeb712a997 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -EINVAL; } - priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL); + priv-clk = devm_clk_get(pdev-dev, internal); if (IS_ERR(priv-clk)) { dev_err(pdev-dev, no clock\n); return PTR_ERR(priv-clk); @@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) 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); -
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 11:37, Russell King - ARM Linux wrote: On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. We _could_, and that would solve this specific issue, but I'd suggest coccinelle is used to locate any other similar instances of this. As I'm allergic to coccinelle (or coccinelle is allergic to me since I never seem to be able to get it to do what I want...) Great. I'd like to avoid adding clk_equal() until we actually need it, and I hope we don't ever need it. We've already got coccinelle in the works to find similar issues and it seems you and I have the same allergies because I can't get it to work for me right now. -- 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 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: On 02/05/15 16:42, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: Actually we can bury the __clk_create_clk() inside __of_clk_get_from_provider(). We should also move __clk_get() into there because right now we have a hole where whoever calls of_clk_get_from_provider() never calls __clk_get() on the clk, leading to possible badness. v2 coming soon. There's some other issues here too... sound/soc/kirkwood/kirkwood-i2s.c: priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL); ... priv-extclk = devm_clk_get(pdev-dev, extclk); if (IS_ERR(priv-extclk)) { ... } else { if (priv-extclk == priv-clk) { devm_clk_put(pdev-dev, priv-extclk); priv-extclk = ERR_PTR(-EINVAL); } else { dev_info(pdev-dev, found external clock\n); clk_prepare_enable(priv-extclk); soc_dai = kirkwood_i2s_dai_extclk; } It should be fine provided your trick is only done for DT clocks, but not for legacy - with legacy, a NULL in the clkdev tables will match both these requests, hence the need to compare the clk_get() return value to tell whether we get the same clock. Are we still talking about of_clk_get_from_provider()? Or are we talking about comparing struct clk pointers? Comparing struct clk pointers, and the implications of the patch changing the clk_get() et.al. to be unique struct clk pointers. From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this return unique clocks. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 12:07, Stephen Boyd wrote: On 02/05/15 11:44, Sylwester Nawrocki wrote: Hi Tomeu, On 23/01/15 12:03, Tomeu Vizoso wrote: int __clk_get(struct clk *clk) { - if (clk) { - if (!try_module_get(clk-owner)) + struct clk_core *core = !clk ? NULL : clk-core; + + if (core) { + if (!try_module_get(core-owner)) return 0; - kref_get(clk-ref); + kref_get(core-ref); } return 1; } -void __clk_put(struct clk *clk) +static void clk_core_put(struct clk_core *core) { struct module *owner; - if (!clk || WARN_ON_ONCE(IS_ERR(clk))) - return; + owner = core-owner; clk_prepare_lock(); - owner = clk-owner; - kref_put(clk-ref, __clk_release); + kref_put(core-ref, __clk_release); clk_prepare_unlock(); module_put(owner); } +void __clk_put(struct clk *clk) +{ + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) + return; + + clk_core_put(clk-core); + kfree(clk); Why do we have kfree() here? clk_get() doesn't allocate the data structure being freed here. What happens if we do clk_get(), clk_put(), clk_get() on same clock? I suspect __clk_free_clk() should be called in __clk_release() callback instead, but then there is an issue of safely getting reference to struct clk from struct clk_core pointer. I tested linux-next on Odroid U3 and booting fails with oopses as below. There is no problems when the above kfree() is commented out. Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't return an allocated clk pointer. Let's fix that. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions of_clk_get_by_clkspec() returns a struct clk pointer but it doesn't create a new handle for the consumers. Instead it just returns whatever the OF clk provider hands out. Let's create a per-user handle here so that clk_put() can properly unlink it and free it when the consumer is done. Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances Reported-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Stephen Boyd sb...@codeaurora.org --- drivers/clk/clkdev.c | 44 +++- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 29a1ab7af4b8..00d747d09b2a 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex); #if defined(CONFIG_OF) defined(CONFIG_COMMON_CLK) -/** - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider - * @clkspec: pointer to a clock specifier data structure - * - * This function looks up a struct clk from the registered list of clock - * providers, an input is a clock specifier data structure as returned - * from the of_parse_phandle_with_args() function call. - */ -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec, + const char *dev_id, const char *con_id) { struct clk *clk; @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) of_clk_lock(); clk = __of_clk_get_from_provider(clkspec); + if (!IS_ERR(clk)) + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id); if (!IS_ERR(clk) !__clk_get(clk)) clk = ERR_PTR(-ENOENT); Actually we can bury the __clk_create_clk() inside __of_clk_get_from_provider(). We should also move __clk_get() into there because right now we have a hole where whoever calls of_clk_get_from_provider() never calls __clk_get() on the clk, leading to possible badness. v2 coming soon. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: Actually we can bury the __clk_create_clk() inside __of_clk_get_from_provider(). We should also move __clk_get() into there because right now we have a hole where whoever calls of_clk_get_from_provider() never calls __clk_get() on the clk, leading to possible badness. v2 coming soon. There's some other issues here too... sound/soc/kirkwood/kirkwood-i2s.c: priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL); ... priv-extclk = devm_clk_get(pdev-dev, extclk); if (IS_ERR(priv-extclk)) { ... } else { if (priv-extclk == priv-clk) { devm_clk_put(pdev-dev, priv-extclk); priv-extclk = ERR_PTR(-EINVAL); } else { dev_info(pdev-dev, found external clock\n); clk_prepare_enable(priv-extclk); soc_dai = kirkwood_i2s_dai_extclk; } It should be fine provided your trick is only done for DT clocks, but not for legacy - with legacy, a NULL in the clkdev tables will match both these requests, hence the need to compare the clk_get() return value to tell whether we get the same clock. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 16:42, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote: Actually we can bury the __clk_create_clk() inside __of_clk_get_from_provider(). We should also move __clk_get() into there because right now we have a hole where whoever calls of_clk_get_from_provider() never calls __clk_get() on the clk, leading to possible badness. v2 coming soon. There's some other issues here too... sound/soc/kirkwood/kirkwood-i2s.c: priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL); ... priv-extclk = devm_clk_get(pdev-dev, extclk); if (IS_ERR(priv-extclk)) { ... } else { if (priv-extclk == priv-clk) { devm_clk_put(pdev-dev, priv-extclk); priv-extclk = ERR_PTR(-EINVAL); } else { dev_info(pdev-dev, found external clock\n); clk_prepare_enable(priv-extclk); soc_dai = kirkwood_i2s_dai_extclk; } It should be fine provided your trick is only done for DT clocks, but not for legacy - with legacy, a NULL in the clkdev tables will match both these requests, hence the need to compare the clk_get() return value to tell whether we get the same clock. Are we still talking about of_clk_get_from_provider()? Or are we talking about comparing struct clk pointers? From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Also, even on DT this could fail if the DT author made internal and extclk map to the same clock provider and cell. -- 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 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Hi Tomeu, On 23/01/15 12:03, Tomeu Vizoso wrote: int __clk_get(struct clk *clk) { - if (clk) { - if (!try_module_get(clk-owner)) + struct clk_core *core = !clk ? NULL : clk-core; + + if (core) { + if (!try_module_get(core-owner)) return 0; - kref_get(clk-ref); + kref_get(core-ref); } return 1; } -void __clk_put(struct clk *clk) +static void clk_core_put(struct clk_core *core) { struct module *owner; - if (!clk || WARN_ON_ONCE(IS_ERR(clk))) - return; + owner = core-owner; clk_prepare_lock(); - owner = clk-owner; - kref_put(clk-ref, __clk_release); + kref_put(core-ref, __clk_release); clk_prepare_unlock(); module_put(owner); } +void __clk_put(struct clk *clk) +{ + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) + return; + + clk_core_put(clk-core); + kfree(clk); Why do we have kfree() here? clk_get() doesn't allocate the data structure being freed here. What happens if we do clk_get(), clk_put(), clk_get() on same clock? I suspect __clk_free_clk() should be called in __clk_release() callback instead, but then there is an issue of safely getting reference to struct clk from struct clk_core pointer. I tested linux-next on Odroid U3 and booting fails with oopses as below. There is no problems when the above kfree() is commented out. +} [1.345850] Unable to handle kernel paging request at virtual address 00200200 [1.349319] pgd = c0004000 [1.352072] [00200200] *pgd= [1.355574] Internal error: Oops: 805 [#1] PREEMPT SMP ARM [1.361035] Modules linked in: [1.364078] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.19.0-rc1-00104-ga251361a-dirty #992 [1.372405] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [1.378483] task: ee00b000 ti: ee088000 task.ti: ee088000 [1.383879] PC is at __clk_put+0x24/0xd0 [1.387773] LR is at clk_prepare_lock+0xc/0xec [1.392198] pc : [c03eef38]lr : [c03ec1f4]psr: 2153 [1.392198] sp : ee089de8 ip : fp : [1.403653] r10: ee02f480 r9 : 0001 r8 : [1.408862] r7 : ee031cc0 r6 : ee089e08 r5 : r4 : ee02f480 [1.415373] r3 : 00100100 r2 : 00200200 r1 : 091e r0 : 0001 [1.421884] Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel [1.429261] Control: 10c5387d Table: 4000404a DAC: 0015 [1.434989] Process swapper/0 (pid: 1, stack limit = 0xee088238) [1.440978] Stack: (0xee089de8 to 0xee08a000) [1.445321] 9de0: ee7c8f14 c03f0ec8 ee089e08 c0718dc8 0001 [1.453480] 9e00: c04ee0f0 ee7e0844 0001 0181 c04edb58 ee2bd320 [1.461639] 9e20: c011dc5c ee16a1e0 c0718dc8 ee16a1e0 ee2bd1e0 [1.469798] 9e40: c0641740 ee16a1e0 ee2bd320 c0718dc8 ee1d3e10 ee1d3e10 [1.477957] 9e60: c0769a88 c0718dc8 c02c3124 c02c310c ee1d3e10 [1.486117] 9e80: c07b4eec c0769a88 c02c1d0c ee1d3e10 c0769a88 ee1d3e44 [1.494276] 9ea0: c07091dc c02c1eb8 c0769a88 c02c1e2c c02c0544 ee005478 ee1676c0 [1.502435] 9ec0: c0769a88 ee3a4e80 c0760ce8 c02c150c c0669b90 c0769a88 c0746cd8 c0769a88 [1.510594] 9ee0: c0746cd8 ee2bc4c0 c0778c00 c02c24e0 c0746cd8 c0746cd8 c07091f0 [1.518753] 9f00: c0008944 c04f405c 0025 ee00b000 6153 c074ab00 [1.526913] 9f20: c074ab90 6153 ef7fca5d c050860c 00b6 c0036b88 [1.535071] 9f40: c065ecc4 c06bc728 0006 0006 c074ab30 ef7fca40 c0739bdc 0006 [1.543231] 9f60: c0718dbc c0778c00 00b6 c0718dc8 c06ed598 c06edd64 0006 0006 [1.551390] 9f80: c06ed598 c003b438 c04e64f4 [1.559549] 9fa0: c04e64fc c000e838 [1.567708] 9fc0: [1.575867] 9fe0: 0013 c0c0c0c0 c0c0c0c0 [1.584045] [c03eef38] (__clk_put) from [c03f0ec8] (of_clk_set_defaults+0xe0/0x2c0) [1.592024] [c03f0ec8] (of_clk_set_defaults) from [c02c3124] (platform_drv_probe+0x18/0xa4) [1.600698] [c02c3124] (platform_drv_probe) from [c02c1d0c] (driver_probe_device+0x10c/0x22c) [1.609549] [c02c1d0c] (driver_probe_device) from [c02c1eb8] (__driver_attach+0x8c/0x90) [1.617968] [c02c1eb8] (__driver_attach) from [c02c0544] (bus_for_each_dev+0x54/0x88) [1.626128] [c02c0544] (bus_for_each_dev) from [c02c150c] (bus_add_driver+0xd4/0x1d0) [1.634286] [c02c150c] (bus_add_driver) from [c02c24e0] (driver_register+0x78/0xf4) [1.642275] [c02c24e0] (driver_register) from [c07091f0] (fimc_md_init+0x14/0x30) [1.650089]
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 11:44, Sylwester Nawrocki wrote: Hi Tomeu, On 23/01/15 12:03, Tomeu Vizoso wrote: int __clk_get(struct clk *clk) { -if (clk) { -if (!try_module_get(clk-owner)) +struct clk_core *core = !clk ? NULL : clk-core; + +if (core) { +if (!try_module_get(core-owner)) return 0; -kref_get(clk-ref); +kref_get(core-ref); } return 1; } -void __clk_put(struct clk *clk) +static void clk_core_put(struct clk_core *core) { struct module *owner; -if (!clk || WARN_ON_ONCE(IS_ERR(clk))) -return; +owner = core-owner; clk_prepare_lock(); -owner = clk-owner; -kref_put(clk-ref, __clk_release); +kref_put(core-ref, __clk_release); clk_prepare_unlock(); module_put(owner); } +void __clk_put(struct clk *clk) +{ +if (!clk || WARN_ON_ONCE(IS_ERR(clk))) +return; + +clk_core_put(clk-core); +kfree(clk); Why do we have kfree() here? clk_get() doesn't allocate the data structure being freed here. What happens if we do clk_get(), clk_put(), clk_get() on same clock? I suspect __clk_free_clk() should be called in __clk_release() callback instead, but then there is an issue of safely getting reference to struct clk from struct clk_core pointer. I tested linux-next on Odroid U3 and booting fails with oopses as below. There is no problems when the above kfree() is commented out. Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't return an allocated clk pointer. Let's fix that. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions of_clk_get_by_clkspec() returns a struct clk pointer but it doesn't create a new handle for the consumers. Instead it just returns whatever the OF clk provider hands out. Let's create a per-user handle here so that clk_put() can properly unlink it and free it when the consumer is done. Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances Reported-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Stephen Boyd sb...@codeaurora.org --- drivers/clk/clkdev.c | 44 +++- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 29a1ab7af4b8..00d747d09b2a 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex); #if defined(CONFIG_OF) defined(CONFIG_COMMON_CLK) -/** - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider - * @clkspec: pointer to a clock specifier data structure - * - * This function looks up a struct clk from the registered list of clock - * providers, an input is a clock specifier data structure as returned - * from the of_parse_phandle_with_args() function call. - */ -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec, +const char *dev_id, const char *con_id) { struct clk *clk; @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) of_clk_lock(); clk = __of_clk_get_from_provider(clkspec); + if (!IS_ERR(clk)) + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id); if (!IS_ERR(clk) !__clk_get(clk)) clk = ERR_PTR(-ENOENT); @@ -54,7 +49,21 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) return clk; } -static struct clk *__of_clk_get(struct device_node *np, int index) +/** + * of_clk_get_by_clkspec() - Lookup a clock form a clock provider + * @clkspec: pointer to a clock specifier data structure + * + * This function looks up a struct clk from the registered list of clock + * providers, an input is a clock specifier data structure as returned + * from the of_parse_phandle_with_args() function call. + */ +struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec) +{ + return __of_clk_get_by_clkspec(clkspec, NULL, __func__); +} + +static struct clk *__of_clk_get(struct device_node *np, int index, + const char *dev_id, const char *con_id) { struct of_phandle_args clkspec; struct clk *clk; @@ -68,7 +77,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index) if (rc) return ERR_PTR(rc); - clk = of_clk_get_by_clkspec(clkspec); + clk = __of_clk_get_by_clkspec(clkspec, dev_id, con_id); of_node_put(clkspec.np); return clk; @@ -76,12 +85,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index) struct clk *of_clk_get(struct device_node *np, int index) { - struct clk *clk = __of_clk_get(np, index); - - if (!IS_ERR(clk)) - clk =
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 05/02/15 20:44, Sylwester Nawrocki wrote: +void __clk_put(struct clk *clk) +{ + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) + return; + + clk_core_put(clk-core); + kfree(clk); Why do we have kfree() here? clk_get() doesn't allocate the data structure being freed here. What happens if we do clk_get(), clk_put(), clk_get() on same clock? I suspect __clk_free_clk() should be called in __clk_release() callback instead, but then there is an issue of safely getting reference to struct clk from struct clk_core pointer. Please ignore this comment, I missed __clk_create_clk() calls in clkdev.c Anyway, in current -next I'm seeing random pointer dereferences while booting Odroid U3, I'll get back to debugging this tomorrow morning. -- Regards, Sylwester -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo t-kri...@ti.com [150203 00:49]: On 02/03/2015 09:03 AM, Tomeu Vizoso wrote: I think you got it right, just wanted to mention that we can and probably should make the clk_get_parent_* calls in the consumer API to return per-user clk instances but that we need to make sure first that callers call clk_put afterwards. This should also allow us to remove the reference to struct clk from clk_hw, which is at best awkward. For the DPLL code it should just be fine to be able to get the current parent index (not parent clock handle), and read a parent clock rate based on an arbitrary index (not just the current one.) I don't think there is any other need for having the clk_ref / clk_bypass clock handles around. I'd like to avoid the situation where the children have know that certain parent index and parent rate means bypass mode for both parent and children. Maybe we can hide that knowledge into some Linux generic PLL code so the children can get the PLL output as a mux clock. For a PLL, there can be three mux clock outputs: refclk*multi/div, bypass clock, or no output. Regards, Tony -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/03/2015 09:03 AM, Tomeu Vizoso wrote: On 02/02/2015 11:41 PM, Mike Turquette wrote: Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. I think you got it right, just wanted to mention that we can and probably should make the clk_get_parent_* calls in the consumer API to return per-user clk instances but that we need to make sure first that callers call clk_put afterwards. This should also allow us to remove the reference to struct clk from clk_hw, which is at best awkward. Regards, For the DPLL code it should just be fine to be able to get the current parent index (not parent clock handle), and read a parent clock rate based on an arbitrary index (not just the current one.) I don't think there is any other need for having the clk_ref / clk_bypass clock handles around. -Tero Tomeu Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Yes, for now that is fine, but feels a bit hacky to me. I don't know honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine but we might want to switch to something like the scheme above. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. Fixed with Stephen's patch from last week. Thanks for dealing with all the breakage so promptly. It has helped a lot! Regards, Mike -Tero Author: Tero Kristo t-kri...@ti.com Date:
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
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo t-kri...@ti.com [150202 11:35]: On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. Thanks Tero, looks like your fix fixes all the issues I'm seeing with commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking on 4430sdp, and off-idle not working for omap3. I could not get the patch to apply, below is what I applied manually. Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with some fuzz on that too. And inn that case, please feel also to add the following for Tomeu's patch: Tested-by: Tony Lindgren t...@atomide.com 8 From: Tero Kristo t-kri...@ti.com Date: Mon, 2 Feb 2015 12:17:00 -0800 Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks DPLL code uses reference and bypass clock pointers for determining runtime properties for these clocks, like parent clock rates. As clock API now returns per-user clock structs, using a global handle in the clock driver code does not work properly anymore. Fix this by using the clk_hw instead, and comparing this against the parents. Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) Signed-off-by: Tero Kristo t-kri...@ti.com [t...@atomide.com: updated to apply] Signed-off-by: Tony Lindgren t...@atomide.com --- a/arch/arm/mach-omap2/dpll3xxx.c +++ b/arch/arm/mach-omap2/dpll3xxx.c @@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) struct clk_hw_omap *clk = to_clk_hw_omap(hw); int r; struct dpll_data *dd; - struct clk *parent; + struct clk_hw *parent; dd = clk-dpll_data; if (!dd) @@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) } } - parent = __clk_get_parent(hw-clk); + parent = __clk_get_hw(__clk_get_parent(hw-clk)); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { - WARN_ON(parent != dd-clk_bypass); + WARN_ON(parent != __clk_get_hw(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { - WARN_ON(parent != dd-clk_ref); + WARN_ON(parent != __clk_get_hw(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } @@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate, if (!dd) return -EINVAL; - if (__clk_get_parent(hw-clk) != dd-clk_ref) + if (__clk_get_hw(__clk_get_parent(hw-clk)) != + __clk_get_hw(dd-clk_ref)) return -EINVAL; if (dd-last_rounded_rate == 0) -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/15 13:24, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. 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. -Stephen Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: dd-clk_ref = of_clk_get(node, 0); dd-clk_bypass = of_clk_get(node, 1); Tony, the same problem affects the FAPLL code which copy/pastes some of the DPLL code. Thoughts? -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Mon, 2 Feb 2015, Stephen Boyd wrote: On 02/01/15 13:24, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. 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? julia -Stephen Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: dd-clk_ref = of_clk_get(node, 0); dd-clk_bypass = of_clk_get(node, 1); Tony, the same problem affects the FAPLL code which copy/pastes some of the DPLL code. Thoughts? -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150202 14:51]: Quoting Tony Lindgren (2015-02-02 12:44:02) Thanks Tero, looks like your fix fixes all the issues I'm seeing with commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking on 4430sdp, and off-idle not working for omap3. I could not get the patch to apply, below is what I applied manually. Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with some fuzz on that too. And inn that case, please feel also to add the following for Tomeu's patch: Tested-by: Tony Lindgren t...@atomide.com Done and done. Things look good in my testing. I've pushed another branch out to the mirrors and hopefully the autobuild/autoboot testing will give us the green light. Thanks I just checked that your updated branch works for me now. This implementation can be revisited probably after 3.19 comes out if Tero doesn't like using clk_hw directly, or if we provide a better interface. Sounds like what Tero is saying also relates to knowing if the parent clock is in bypass mode or not in addition to the parent rate. Regards, Tony -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 14:41, Mike Turquette wrote: Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. The plan is to make clk_get_parent_by_index() return a clk_hw pointer instead of a clk pointer (probably with a new clk_get_parent_hw_by_index() API). Then drivers that are clk providers can deal in struct clk_hw and clk consumers can deal in struct clk, nicely splitting the API between consumers and providers on the structures they use to interact with the framework. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Yes, for now that is fine, but feels a bit hacky to me. I don't know honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine but we might want to switch to something like the scheme above. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. Fixed with Stephen's patch from last week. Thanks for dealing with all the breakage so promptly. It has helped a lot! Regards, Mike -Tero Author: Tero Kristo t-kri...@ti.com Date: Mon Feb 2 17:19:17 2015 +0200 ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks DPLL code uses reference and bypass clock pointers for determining runtime properties for these clocks, like parent clock rates. As clock API now returns per-user clock structs, using a global handle in the clock driver code does not work properly anymore. Fix this by using the clk_hw instead, and comparing this against the parents. Signed-off-by: Tero Kristo t-kri...@ti.com Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) diff --git
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tony Lindgren (2015-02-02 12:44:02) * Tero Kristo t-kri...@ti.com [150202 11:35]: On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. Thanks Tero, looks like your fix fixes all the issues I'm seeing with commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking on 4430sdp, and off-idle not working for omap3. I could not get the patch to apply, below is what I applied manually. Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with some fuzz on that too. And inn that case, please feel also to add the following for Tomeu's patch: Tested-by: Tony Lindgren t...@atomide.com Done and done. Things look good in my testing. I've pushed another branch out to the mirrors and hopefully the autobuild/autoboot testing will give us the green light. This implementation can be revisited probably after 3.19 comes out if Tero doesn't like using clk_hw directly, or if we provide a better interface. Thanks, Mike 8 From: Tero Kristo t-kri...@ti.com Date: Mon, 2 Feb 2015 12:17:00 -0800 Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks DPLL code uses reference and bypass clock pointers for determining runtime properties for these clocks, like parent clock rates. As clock API now returns per-user clock structs, using a global handle in the clock driver code does not work properly anymore. Fix this by using the clk_hw instead, and comparing this against the parents. Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) Signed-off-by: Tero Kristo t-kri...@ti.com [t...@atomide.com: updated to apply] Signed-off-by: Tony Lindgren t...@atomide.com --- a/arch/arm/mach-omap2/dpll3xxx.c +++ b/arch/arm/mach-omap2/dpll3xxx.c @@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) struct clk_hw_omap *clk = to_clk_hw_omap(hw); int r; struct dpll_data *dd; - struct clk *parent; + struct clk_hw *parent; dd = clk-dpll_data; if (!dd) @@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) } } - parent = __clk_get_parent(hw-clk); + parent = __clk_get_hw(__clk_get_parent(hw-clk)); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { - WARN_ON(parent != dd-clk_bypass); + WARN_ON(parent != __clk_get_hw(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { - WARN_ON(parent != dd-clk_ref); + WARN_ON(parent != __clk_get_hw(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } @@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate, if (!dd) return -EINVAL; - if (__clk_get_parent(hw-clk) != dd-clk_ref) + if (__clk_get_hw(__clk_get_parent(hw-clk)) != + __clk_get_hw(dd-clk_ref)) return -EINVAL; if (dd-last_rounded_rate == 0) -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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. -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
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 -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/2015 11:41 PM, Mike Turquette wrote: Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. I think you got it right, just wanted to mention that we can and probably should make the clk_get_parent_* calls in the consumer API to return per-user clk instances but that we need to make sure first that callers call clk_put afterwards. This should also allow us to remove the reference to struct clk from clk_hw, which is at best awkward. Regards, Tomeu Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Yes, for now that is fine, but feels a bit hacky to me. I don't know honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine but we might want to switch to something like the scheme above. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. Fixed with Stephen's patch from last week. Thanks for dealing with all the breakage so promptly. It has helped a lot! Regards, Mike -Tero Author: Tero Kristo t-kri...@ti.com Date: Mon Feb 2 17:19:17 2015 +0200 ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks DPLL code uses reference and bypass clock pointers for determining runtime properties for these clocks, like parent clock rates. As clock API now returns
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150201 13:27]: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. Also on at least omap4 like I posted. So sounds like the check for WARN is wrong but harmless. Paul Tero, what do you want to do about that? I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: dd-clk_ref = of_clk_get(node, 0); dd-clk_bypass = of_clk_get(node, 1); Tony, the same problem affects the FAPLL code which copy/pastes some of the DPLL code. Thoughts? Not seeing these warnings with dm186x as fapll.c does not use dpll3xxx.c. This is because of the way the PLL's child synthesizers need to also access the PLL registers for power and bypass mode. Not related to the $subject bug, but to me it seems that we could possibly have Linux generic PLL code if we add support for parent_in_bypass_mode in addition to the parent_rate. This is because the PLL can in theory generate the same rate both in bypass mode and regular mode so parent_rate is not enough to tell it to the child synthesizers. Not sure how the PLL registers enabling and disabling it's children should be handled, maybe regmap would work there. Regards, Tony -- 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: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/2015 11:24 PM, Mike Turquette wrote: Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. Shall I change the DPLL code to check against clk_hw pointers or what is the preferred approach here? The patch at the end does this and fixes the dpll related warnings. Btw, the rate constraints patch broke boot for me completely, but sounds like you reverted it already. -Tero Author: Tero Kristo t-kri...@ti.com Date: Mon Feb 2 17:19:17 2015 +0200 ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks DPLL code uses reference and bypass clock pointers for determining runtime properties for these clocks, like parent clock rates. As clock API now returns per-user clock structs, using a global handle in the clock driver code does not work properly anymore. Fix this by using the clk_hw instead, and comparing this against the parents. Signed-off-by: Tero Kristo t-kri...@ti.com Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c index c2da2a0..49752d7 100644 --- a/arch/arm/mach-omap2/dpll3xxx.c +++ b/arch/arm/mach-omap2/dpll3xxx.c @@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) struct clk_hw_omap *clk = to_clk_hw_omap(hw); int r; struct dpll_data *dd; - struct clk *parent; + struct clk_hw *parent; dd = clk-dpll_data; if (!dd) @@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw) } } - parent = __clk_get_parent(hw-clk); + parent = __clk_get_hw(__clk_get_parent(hw-clk)); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { - WARN_ON(parent != dd-clk_bypass); + WARN_ON(parent != __clk_get_hw(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { - WARN_ON(parent != dd-clk_ref); + WARN_ON(parent != __clk_get_hw(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } @@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate, if (!dd) return -EINVAL; - if (__clk_get_parent(hw-clk) != dd-clk_ref) + if (__clk_get_hw(__clk_get_parent(hw-clk))
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tomeu Vizoso (2015-01-23 03:03:30) Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Tero, Paul Tony, Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and struct dpll_data, namely this snippet from arch/arm/mach-omap2/dpll3xxx.c: parent = __clk_get_parent(hw-clk); if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) { WARN(parent != dd-clk_bypass, here0, parent name is %s, bypass name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_bypass)); r = _omap3_noncore_dpll_bypass(clk); } else { WARN(parent != dd-clk_ref, here1, parent name is %s, ref name is %s\n, __clk_get_name(parent), __clk_get_name(dd-clk_ref)); r = _omap3_noncore_dpll_lock(clk); } struct dpll_data has members clk_ref and clk_bypass which are struct clk pointers. This was always a bit of a violation of the clk.h contract since drivers are not supposed to deref struct clk pointers. Now that we generate unique pointers for each call to clk_get (clk_ref clk_bypass are populated by of_clk_get in ti_clk_register_dpll) then the pointer comparisons above will never be equal (even if they resolve down to the same struct clk_core). I added the verbose traces to the WARNs above to illustrate the point: the names are always the same but the pointers differ. AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: dd-clk_ref = of_clk_get(node, 0); dd-clk_bypass = of_clk_get(node, 1); Tony, the same problem affects the FAPLL code which copy/pastes some of the DPLL code. Thoughts? Regards, Mike -- 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
[PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Moves clock state to struct clk_core, but takes care to change as little API as possible. struct clk_hw still has a pointer to a struct clk, which is the implementation's per-user clk instance, for backwards compatibility. The struct clk that clk_get_parent() returns isn't owned by the caller, but by the clock implementation, so the former shouldn't call clk_put() on it. Because some boards in mach-omap2 still register clocks statically, their clock registration had to be updated to take into account that the clock information is stored in struct clk_core now. Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com Reviewed-by: Stephen Boyd sb...@codeaurora.org --- v13:* Split some lines to 80 columns * Add dev_id parameter to __of_clk_get_by_name so it can be called by clk_get without losing information v12:* Move __clk_create_clk so it's later next to __clk_free_clk * NULLify a just-free'd pointer * Check the value returned by __clk_create_clk in clk_register v11:* Allow for clk_set_parent to be called with a NULL parent v10:* Add more missing NULL checks * Remove __clk_reparent as it's now unused * Remove clk_core_round_rate as it's now unused * Call nolock variants from __clk_mux_determine_rate v9: * Add missing NULL checks * Remove __clk_prepare and __clk_unprepare as they are unused now v7: * Add stub for __of_clk_get_by_name to fix builds without OF v6: * Guard against NULL pointer v4: * Remove unused function __clk_core_to_clk * Use core more often as the name for struct clk_core* variables * Make sure we don't lose information about the caller in of_clk_get_* v3: * Rebase on top of linux-next 20141009 v2: * Remove exported functions that aren't really used outside clk.c * Rename new internal functions to clk_core_ prefix * Remove redundant checks for error pointers in *_get_parent * Change __clk_create_clk to take a struct clk_hw instead * Match the original error behavior in clk_get_sys --- arch/arm/mach-omap2/cclock3xxx_data.c | 111 -- arch/arm/mach-omap2/clock.h | 11 +- arch/arm/mach-omap2/clock_common_data.c | 5 +- drivers/clk/clk.c | 621 drivers/clk/clk.h | 5 + drivers/clk/clkdev.c| 84 - include/linux/clk-private.h | 35 +- include/linux/clk-provider.h| 12 +- 8 files changed, 589 insertions(+), 295 deletions(-) diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c index 644ff32..adb4e64 100644 --- a/arch/arm/mach-omap2/cclock3xxx_data.c +++ b/arch/arm/mach-omap2/cclock3xxx_data.c @@ -82,7 +82,7 @@ DEFINE_CLK_MUX(osc_sys_ck, osc_sys_ck_parent_names, NULL, 0x0, OMAP3430_PRM_CLKSEL, OMAP3430_SYS_CLKIN_SEL_SHIFT, OMAP3430_SYS_CLKIN_SEL_WIDTH, 0x0, NULL); -DEFINE_CLK_DIVIDER(sys_ck, osc_sys_ck, osc_sys_ck, 0x0, +DEFINE_CLK_DIVIDER(sys_ck, osc_sys_ck, osc_sys_ck_core, 0x0, OMAP3430_PRM_CLKSRC_CTRL, OMAP_SYSCLKDIV_SHIFT, OMAP_SYSCLKDIV_WIDTH, CLK_DIVIDER_ONE_BASED, NULL); @@ -132,7 +132,7 @@ static struct clk_hw_omap dpll3_ck_hw = { DEFINE_STRUCT_CLK(dpll3_ck, dpll3_ck_parent_names, dpll3_ck_ops); -DEFINE_CLK_DIVIDER(dpll3_m2_ck, dpll3_ck, dpll3_ck, 0x0, +DEFINE_CLK_DIVIDER(dpll3_m2_ck, dpll3_ck, dpll3_ck_core, 0x0, OMAP_CM_REGADDR(PLL_MOD, CM_CLKSEL1), OMAP3430_CORE_DPLL_CLKOUT_DIV_SHIFT, OMAP3430_CORE_DPLL_CLKOUT_DIV_WIDTH, @@ -149,12 +149,12 @@ static const struct clk_ops core_ck_ops = {}; DEFINE_STRUCT_CLK_HW_OMAP(core_ck, NULL); DEFINE_STRUCT_CLK(core_ck, core_ck_parent_names, core_ck_ops); -DEFINE_CLK_DIVIDER(l3_ick, core_ck, core_ck, 0x0, +DEFINE_CLK_DIVIDER(l3_ick, core_ck, core_ck_core, 0x0, OMAP_CM_REGADDR(CORE_MOD, CM_CLKSEL), OMAP3430_CLKSEL_L3_SHIFT, OMAP3430_CLKSEL_L3_WIDTH, CLK_DIVIDER_ONE_BASED, NULL); -DEFINE_CLK_DIVIDER(l4_ick, l3_ick, l3_ick, 0x0, +DEFINE_CLK_DIVIDER(l4_ick, l3_ick, l3_ick_core, 0x0, OMAP_CM_REGADDR(CORE_MOD, CM_CLKSEL), OMAP3430_CLKSEL_L4_SHIFT, OMAP3430_CLKSEL_L4_WIDTH, CLK_DIVIDER_ONE_BASED, NULL); @@ -275,9 +275,9 @@ static struct clk_hw_omap dpll1_ck_hw = { DEFINE_STRUCT_CLK(dpll1_ck, dpll3_ck_parent_names, dpll1_ck_ops); -DEFINE_CLK_FIXED_FACTOR(dpll1_x2_ck, dpll1_ck, dpll1_ck, 0x0, 2, 1); +DEFINE_CLK_FIXED_FACTOR(dpll1_x2_ck, dpll1_ck, dpll1_ck_core, 0x0, 2, 1); -DEFINE_CLK_DIVIDER(dpll1_x2m2_ck, dpll1_x2_ck, dpll1_x2_ck, 0x0, +DEFINE_CLK_DIVIDER(dpll1_x2m2_ck, dpll1_x2_ck, dpll1_x2_ck_core, 0x0, OMAP_CM_REGADDR(MPU_MOD, OMAP3430_CM_CLKSEL2_PLL), OMAP3430_MPU_DPLL_CLKOUT_DIV_SHIFT,