* [RFC] clk: add flags to distinguish xtal clocks @ 2013-07-04 21:05 Luciano Coelho [not found] ` <20130704222538.10823.2559@quantum> 2013-07-05 13:12 ` [RFC] " James Hogan 0 siblings, 2 replies; 22+ messages in thread From: Luciano Coelho @ 2013-07-04 21:05 UTC (permalink / raw) To: mturquette; +Cc: linux-arm-kernel, linux-kernel, balbi, coelho Add a flag that indicate whether the clock is a crystal or not. Since no clocks set this flag right now, include an additional flag that indicates whether the type is set or not. If the CLK_IS_TYPE_DEFINED flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined. This ensures backwards compatibility. Additionally, parse a new device tree binding in clk-fixed-rate to set this flag. Signed-off-by: Luciano Coelho <coelho@ti.com> --- I'm not familiar with the common clock framework and I'm not entirely sure the flags can be used in such a way, but to me it looks reasonable, since some clock consumers may need to know what type of clock is being provided. Specifically, the wl12xx firmware needs to know if the clock is XTAL or not to handle the stabilization and boosts properly. My main idea is that I need to pass this information in the device tree definition of the clocks, so that the driver can pass this information on to the firmware. Please let me know if this looks ok or not. If not, please let me know if you have any other ideas on how to solve my problem (of knowing whether the clock attached to the WiLink chip is XTAL or not). drivers/clk/clk-fixed-rate.c | 6 +++++- include/linux/clk-provider.h | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c index dc58fbd..4003a82 100644 --- a/drivers/clk/clk-fixed-rate.c +++ b/drivers/clk/clk-fixed-rate.c @@ -90,13 +90,17 @@ void of_fixed_clk_setup(struct device_node *node) struct clk *clk; const char *clk_name = node->name; u32 rate; + unsigned long flags = CLK_IS_ROOT; if (of_property_read_u32(node, "clock-frequency", &rate)) return; + if (of_property_read_bool(node, "clock-xtal")) + flags |= CLK_IS_TYPE_DEFINED | CLK_IS_TYPE_XTAL; + of_property_read_string(node, "clock-output-names", &clk_name); - clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate); + clk = clk_register_fixed_rate(NULL, clk_name, NULL, flags, rate); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk); } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 1186098..034320b 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -27,6 +27,8 @@ #define CLK_IS_ROOT BIT(4) /* root clk, has no parent */ #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ #define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ +#define CLK_IS_TYPE_DEFINED BIT(7) /* the clock type is defined */ +#define CLK_IS_TYPE_XTAL BIT(8) /* this is a crystal clock */ struct clk_hw; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
[parent not found: <20130704222538.10823.2559@quantum>]
* Re: [RFC] clk: add flags to distinguish xtal clocks [not found] ` <20130704222538.10823.2559@quantum> @ 2013-07-04 22:37 ` Luciano Coelho [not found] ` <20130704231953.10823.94331@quantum> 0 siblings, 1 reply; 22+ messages in thread From: Luciano Coelho @ 2013-07-04 22:37 UTC (permalink / raw) To: Mike Turquette; +Cc: linux-arm-kernel, linux-kernel, balbi On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > Quoting Luciano Coelho (2013-07-04 14:05:12) > > Add a flag that indicate whether the clock is a crystal or not. Since > > no clocks set this flag right now, include an additional flag that > > indicates whether the type is set or not. If the CLK_IS_TYPE_DEFINED > > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined. > > This ensures backwards compatibility. > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > this flag. > > > > Signed-off-by: Luciano Coelho <coelho@ti.com> > > --- > > > > I'm not familiar with the common clock framework and I'm not > > entirely sure the flags can be used in such a way, but to me it looks > > reasonable, since some clock consumers may need to know what type of > > clock is being provided. > > > > Specifically, the wl12xx firmware needs to know if the clock is XTAL > > or not to handle the stabilization and boosts properly. > > Hi Luciano, Hi Mike, > I'd like clarification on one point. Is the same clock input signal used > on the wifi chip? What I mean is, are there multiple clock inputs and > XTAL is one, and not-XTAL is another? This wifi chip can work with a few different clocks and some of them are XTAL and others are not. What the chip's firmware can use is one of these: 19.2MHz (not XTAL) 26.0MHz (not XTAL) 26.0MHz (XTAL) 38.4MHz (not XTAL) 38.4MHz (XTAL) 52.0MHz (not XTAL) It treats the XTAL clocks differently (but I don't really understand enough of the details), so the driver needs to configure the firmware according to these values. > Or is it the same clock input and basically the problem is that you need > to know what kind of waveform to expect (e.g. square versus sine)? It's the same clock input in the chip's perspective. One clock input that can be any of the combinations I mentioned above. Again, I'm not familiar with clocks, so I guess the square vs. sine explanation is plausible. What I could see in the firmware is that it handles the clocks differently if they're xtal or not. -- Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20130704231953.10823.94331@quantum>]
* Re: [RFC] clk: add flags to distinguish xtal clocks [not found] ` <20130704231953.10823.94331@quantum> @ 2013-07-05 7:54 ` Luciano Coelho 2013-07-29 13:50 ` Luciano Coelho 0 siblings, 1 reply; 22+ messages in thread From: Luciano Coelho @ 2013-07-05 7:54 UTC (permalink / raw) To: Mike Turquette; +Cc: linux-arm-kernel, linux-kernel, balbi On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > Quoting Luciano Coelho (2013-07-04 15:37:45) > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > Or is it the same clock input and basically the problem is that you need > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > It's the same clock input in the chip's perspective. One clock input > > that can be any of the combinations I mentioned above. Again, I'm not > > familiar with clocks, so I guess the square vs. sine explanation is > > plausible. What I could see in the firmware is that it handles the > > clocks differently if they're xtal or not. > > OMAP has a similar thing where sys_clkin (the fast reference clock for > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > the rates matter. Right, this part is easy and I already have the code for that. What I'm missing is a way to pass this XTAL flag to the chip. > In your case you need some extra metadata to know what to do. I'm really > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > take. It would be best to know if the waveform is what you really need > to know, or perhaps something else. For instance you might be affected > by some clock signal stabilization time. Can you talk to your hardware > guys and figure it out? I'd rather model the actual needs instead of > just tossing a flag in there. I get your point. I have tried to investigate how this flag is used by the firmware and I could see that it is used to set different "buffer gains" and "delays" when waking up (I guess this means when the clock is starting, so probably related to stabilization time). They specify two "modes", "boost" and "normal" and use different delay values for each. As I said, I don't know almost anything about clocks, so all this doesn't make much sense to me. But maybe you can get an idea? -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-07-05 7:54 ` Luciano Coelho @ 2013-07-29 13:50 ` Luciano Coelho [not found] ` <20131007074424.7445.52119@quantum> 0 siblings, 1 reply; 22+ messages in thread From: Luciano Coelho @ 2013-07-29 13:50 UTC (permalink / raw) To: Mike Turquette; +Cc: linux-arm-kernel, linux-kernel, balbi, James Hogan Hi Mike, On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote: > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > > Quoting Luciano Coelho (2013-07-04 15:37:45) > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > > Or is it the same clock input and basically the problem is that you need > > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > > > It's the same clock input in the chip's perspective. One clock input > > > that can be any of the combinations I mentioned above. Again, I'm not > > > familiar with clocks, so I guess the square vs. sine explanation is > > > plausible. What I could see in the firmware is that it handles the > > > clocks differently if they're xtal or not. > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > > the rates matter. > > Right, this part is easy and I already have the code for that. What I'm > missing is a way to pass this XTAL flag to the chip. > > > > In your case you need some extra metadata to know what to do. I'm really > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > > take. It would be best to know if the waveform is what you really need > > to know, or perhaps something else. For instance you might be affected > > by some clock signal stabilization time. Can you talk to your hardware > > guys and figure it out? I'd rather model the actual needs instead of > > just tossing a flag in there. > > I get your point. I have tried to investigate how this flag is used by > the firmware and I could see that it is used to set different "buffer > gains" and "delays" when waking up (I guess this means when the clock is > starting, so probably related to stabilization time). They specify two > "modes", "boost" and "normal" and use different delay values for each. I tried but I couldn't find any more information on how exactly this works. But since this change is really simple and there seems to be other people who need the same information, couldn't we add it as is and try to figure out more specific information about the clocks later on? Even if XTAL is not that useful if we know the other details, at least it wouldn't hurt to have the flag there anyway. -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20131007074424.7445.52119@quantum>]
* Re: [RFC] clk: add flags to distinguish xtal clocks [not found] ` <20131007074424.7445.52119@quantum> @ 2013-10-08 15:27 ` Felipe Balbi 2013-10-16 10:24 ` Luca Coelho 0 siblings, 1 reply; 22+ messages in thread From: Felipe Balbi @ 2013-10-08 15:27 UTC (permalink / raw) To: Mike Turquette Cc: linux-arm-kernel, linux-kernel, balbi, James Hogan, Luciano Coelho [-- Attachment #1: Type: text/plain, Size: 3014 bytes --] Hi, Fixing Luca's address since he left TI On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote: > Quoting Luciano Coelho (2013-07-29 06:50:42) > > Hi Mike, > > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote: > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > > > > Quoting Luciano Coelho (2013-07-04 15:37:45) > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > > > > Or is it the same clock input and basically the problem is that you need > > > > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > > > > > > > It's the same clock input in the chip's perspective. One clock input > > > > > that can be any of the combinations I mentioned above. Again, I'm not > > > > > familiar with clocks, so I guess the square vs. sine explanation is > > > > > plausible. What I could see in the firmware is that it handles the > > > > > clocks differently if they're xtal or not. > > > > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > > > > the rates matter. > > > > > > Right, this part is easy and I already have the code for that. What I'm > > > missing is a way to pass this XTAL flag to the chip. > > > > > > > > > > In your case you need some extra metadata to know what to do. I'm really > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > > > > take. It would be best to know if the waveform is what you really need > > > > to know, or perhaps something else. For instance you might be affected > > > > by some clock signal stabilization time. Can you talk to your hardware > > > > guys and figure it out? I'd rather model the actual needs instead of > > > > just tossing a flag in there. > > > > > > I get your point. I have tried to investigate how this flag is used by > > > the firmware and I could see that it is used to set different "buffer > > > gains" and "delays" when waking up (I guess this means when the clock is > > > starting, so probably related to stabilization time). They specify two > > > "modes", "boost" and "normal" and use different delay values for each. > > > > I tried but I couldn't find any more information on how exactly this > > works. But since this change is really simple and there seems to be > > other people who need the same information, couldn't we add it as is and > > try to figure out more specific information about the clocks later on? > > > > Even if XTAL is not that useful if we know the other details, at least > > it wouldn't hurt to have the flag there anyway. > > Luca, > > By any chance did you come to a different solution for this problem? I > can take the patch, but I do not feel like we're solving the right > problem the right way. > > If not I will take it for 3.13. > > Regards, > Mike > > > > > -- > > Cheers, > > Luca. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-10-08 15:27 ` Felipe Balbi @ 2013-10-16 10:24 ` Luca Coelho 2013-10-23 9:24 ` Mike Turquette 0 siblings, 1 reply; 22+ messages in thread From: Luca Coelho @ 2013-10-16 10:24 UTC (permalink / raw) To: Mike Turquette, balbi; +Cc: linux-arm-kernel, linux-kernel, James Hogan Hi, Sorry for the delayed response. On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote: > Fixing Luca's address since he left TI Thanks, Felipe! I wouldn't have seen this otherwise. > On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote: > > Quoting Luciano Coelho (2013-07-29 06:50:42) > > > Hi Mike, > > > > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote: > > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > > > > > Quoting Luciano Coelho (2013-07-04 15:37:45) > > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > > > > > Or is it the same clock input and basically the problem is that you need > > > > > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > > > > > > > > > It's the same clock input in the chip's perspective. One clock input > > > > > > that can be any of the combinations I mentioned above. Again, I'm not > > > > > > familiar with clocks, so I guess the square vs. sine explanation is > > > > > > plausible. What I could see in the firmware is that it handles the > > > > > > clocks differently if they're xtal or not. > > > > > > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for > > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > > > > > the rates matter. > > > > > > > > Right, this part is easy and I already have the code for that. What I'm > > > > missing is a way to pass this XTAL flag to the chip. > > > > > > > > > > > > > In your case you need some extra metadata to know what to do. I'm really > > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > > > > > take. It would be best to know if the waveform is what you really need > > > > > to know, or perhaps something else. For instance you might be affected > > > > > by some clock signal stabilization time. Can you talk to your hardware > > > > > guys and figure it out? I'd rather model the actual needs instead of > > > > > just tossing a flag in there. > > > > > > > > I get your point. I have tried to investigate how this flag is used by > > > > the firmware and I could see that it is used to set different "buffer > > > > gains" and "delays" when waking up (I guess this means when the clock is > > > > starting, so probably related to stabilization time). They specify two > > > > "modes", "boost" and "normal" and use different delay values for each. > > > > > > I tried but I couldn't find any more information on how exactly this > > > works. But since this change is really simple and there seems to be > > > other people who need the same information, couldn't we add it as is and > > > try to figure out more specific information about the clocks later on? > > > > > > Even if XTAL is not that useful if we know the other details, at least > > > it wouldn't hurt to have the flag there anyway. > > > > Luca, > > > > By any chance did you come to a different solution for this problem? I > > can take the patch, but I do not feel like we're solving the right > > problem the right way. Unfortunately I didn't find any better solution for this. From the WiLink firmware's point of view, there was not much information about the details of stabilization time requirements and such. > > If not I will take it for 3.13. That would be great! As I mentioned above, the XTAL/non-XTAL flag wouldn't hurt, even if in most cases it may not provide the necessary details for the clock code. But at least for the WiLink hardware it is very useful. ;) -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-10-16 10:24 ` Luca Coelho @ 2013-10-23 9:24 ` Mike Turquette 2013-10-23 11:35 ` Luca Coelho 0 siblings, 1 reply; 22+ messages in thread From: Mike Turquette @ 2013-10-23 9:24 UTC (permalink / raw) To: Luca Coelho, balbi; +Cc: linux-arm-kernel, linux-kernel, James Hogan Quoting Luca Coelho (2013-10-16 03:24:27) > Hi, > > Sorry for the delayed response. > > On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote: > > Fixing Luca's address since he left TI > > Thanks, Felipe! I wouldn't have seen this otherwise. > > > > On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote: > > > Quoting Luciano Coelho (2013-07-29 06:50:42) > > > > Hi Mike, > > > > > > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote: > > > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > > > > > > Quoting Luciano Coelho (2013-07-04 15:37:45) > > > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > > > > > > Or is it the same clock input and basically the problem is that you need > > > > > > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > > > > > > > > > > > It's the same clock input in the chip's perspective. One clock input > > > > > > > that can be any of the combinations I mentioned above. Again, I'm not > > > > > > > familiar with clocks, so I guess the square vs. sine explanation is > > > > > > > plausible. What I could see in the firmware is that it handles the > > > > > > > clocks differently if they're xtal or not. > > > > > > > > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for > > > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > > > > > > the rates matter. > > > > > > > > > > Right, this part is easy and I already have the code for that. What I'm > > > > > missing is a way to pass this XTAL flag to the chip. > > > > > > > > > > > > > > > > In your case you need some extra metadata to know what to do. I'm really > > > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > > > > > > take. It would be best to know if the waveform is what you really need > > > > > > to know, or perhaps something else. For instance you might be affected > > > > > > by some clock signal stabilization time. Can you talk to your hardware > > > > > > guys and figure it out? I'd rather model the actual needs instead of > > > > > > just tossing a flag in there. > > > > > > > > > > I get your point. I have tried to investigate how this flag is used by > > > > > the firmware and I could see that it is used to set different "buffer > > > > > gains" and "delays" when waking up (I guess this means when the clock is > > > > > starting, so probably related to stabilization time). They specify two > > > > > "modes", "boost" and "normal" and use different delay values for each. > > > > > > > > I tried but I couldn't find any more information on how exactly this > > > > works. But since this change is really simple and there seems to be > > > > other people who need the same information, couldn't we add it as is and > > > > try to figure out more specific information about the clocks later on? > > > > > > > > Even if XTAL is not that useful if we know the other details, at least > > > > it wouldn't hurt to have the flag there anyway. > > > > > > Luca, > > > > > > By any chance did you come to a different solution for this problem? I > > > can take the patch, but I do not feel like we're solving the right > > > problem the right way. > > Unfortunately I didn't find any better solution for this. From the > WiLink firmware's point of view, there was not much information about > the details of stabilization time requirements and such. > > > > > If not I will take it for 3.13. > > That would be great! As I mentioned above, the XTAL/non-XTAL flag > wouldn't hurt, even if in most cases it may not provide the necessary > details for the clock code. But at least for the WiLink hardware it is > very useful. ;) Why is the CLK_IS_TYPE_DEFINED necessary? Also we're adding a property to the fixed-rate binding so Documentation/devicetree/bindings/clock/fixed-clock.txt needs to be updated. Regards, Mike > > -- > Cheers, > Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-10-23 9:24 ` Mike Turquette @ 2013-10-23 11:35 ` Luca Coelho 2013-11-08 18:00 ` [PATCH] " Felipe Balbi 0 siblings, 1 reply; 22+ messages in thread From: Luca Coelho @ 2013-10-23 11:35 UTC (permalink / raw) To: Mike Turquette; +Cc: balbi, linux-arm-kernel, linux-kernel, James Hogan On Wed, 2013-10-23 at 02:24 -0700, Mike Turquette wrote: > Quoting Luca Coelho (2013-10-16 03:24:27) > > Hi, > > > > Sorry for the delayed response. > > > > On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote: > > > Fixing Luca's address since he left TI > > > > Thanks, Felipe! I wouldn't have seen this otherwise. > > > > > > > On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote: > > > > Quoting Luciano Coelho (2013-07-29 06:50:42) > > > > > Hi Mike, > > > > > > > > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote: > > > > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote: > > > > > > > Quoting Luciano Coelho (2013-07-04 15:37:45) > > > > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote: > > > > > > > > > Or is it the same clock input and basically the problem is that you need > > > > > > > > > to know what kind of waveform to expect (e.g. square versus sine)? > > > > > > > > > > > > > > > > It's the same clock input in the chip's perspective. One clock input > > > > > > > > that can be any of the combinations I mentioned above. Again, I'm not > > > > > > > > familiar with clocks, so I guess the square vs. sine explanation is > > > > > > > > plausible. What I could see in the firmware is that it handles the > > > > > > > > clocks differently if they're xtal or not. > > > > > > > > > > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for > > > > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only > > > > > > > the rates matter. > > > > > > > > > > > > Right, this part is easy and I already have the code for that. What I'm > > > > > > missing is a way to pass this XTAL flag to the chip. > > > > > > > > > > > > > > > > > > > In your case you need some extra metadata to know what to do. I'm really > > > > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can > > > > > > > take. It would be best to know if the waveform is what you really need > > > > > > > to know, or perhaps something else. For instance you might be affected > > > > > > > by some clock signal stabilization time. Can you talk to your hardware > > > > > > > guys and figure it out? I'd rather model the actual needs instead of > > > > > > > just tossing a flag in there. > > > > > > > > > > > > I get your point. I have tried to investigate how this flag is used by > > > > > > the firmware and I could see that it is used to set different "buffer > > > > > > gains" and "delays" when waking up (I guess this means when the clock is > > > > > > starting, so probably related to stabilization time). They specify two > > > > > > "modes", "boost" and "normal" and use different delay values for each. > > > > > > > > > > I tried but I couldn't find any more information on how exactly this > > > > > works. But since this change is really simple and there seems to be > > > > > other people who need the same information, couldn't we add it as is and > > > > > try to figure out more specific information about the clocks later on? > > > > > > > > > > Even if XTAL is not that useful if we know the other details, at least > > > > > it wouldn't hurt to have the flag there anyway. > > > > > > > > Luca, > > > > > > > > By any chance did you come to a different solution for this problem? I > > > > can take the patch, but I do not feel like we're solving the right > > > > problem the right way. > > > > Unfortunately I didn't find any better solution for this. From the > > WiLink firmware's point of view, there was not much information about > > the details of stabilization time requirements and such. > > > > > > > > If not I will take it for 3.13. > > > > That would be great! As I mentioned above, the XTAL/non-XTAL flag > > wouldn't hurt, even if in most cases it may not provide the necessary > > details for the clock code. But at least for the WiLink hardware it is > > very useful. ;) > > Why is the CLK_IS_TYPE_DEFINED necessary? This is just for backwards compatibility. Currently all the other clocks don't set the XTAL/non-XTAL flag (even if they *are* XTAL), so if the CLK_IS_TYPE_DEFINED is not set, we know we can't trust the XTAL/no-XTAL flag. > Also we're adding a property to the fixed-rate binding so > Documentation/devicetree/bindings/clock/fixed-clock.txt needs to be > updated. Yeah, you're right. Unfortunately, I won't have the time to resubmit this patch any time soon. :( -- Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] clk: add flags to distinguish xtal clocks 2013-10-23 11:35 ` Luca Coelho @ 2013-11-08 18:00 ` Felipe Balbi 2013-11-08 19:16 ` Luca Coelho ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Felipe Balbi @ 2013-11-08 18:00 UTC (permalink / raw) To: mturquette Cc: Linux ARM Kernel Mailing List, Linux OMAP Mailing List, Linux Kernel Mailing List, james.hogan, luca, Felipe Balbi From: Luciano Coelho <luca@coelho.fi> Add a flag that indicate whether the clock is a crystal or not. Additionally, parse a new device tree binding in clk-fixed-rate to set this flag. If clock-xtal isn't set, the clock framework will assume clock to be generated by an oscillator. There's only one user for this binding right now which is Texas Instruments' WiLink devices which need to know details about the clock in order to initialize the underlying WiFi HW correctly. Signed-off-by: Luciano Coelho <luca@coelho.fi> Signed-off-by: Felipe Balbi <balbi@ti.com> --- Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag isn't there, default behavior will be taken. Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 + drivers/clk/clk-fixed-rate.c | 6 +++++- include/linux/clk-provider.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt index 0b1fe78..3036dfe 100644 --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt @@ -12,6 +12,7 @@ Required properties: Optional properties: - gpios : From common gpio binding; gpio connection to clock enable pin. - clock-output-names : From common clock binding. +- clock-xtal: true when a clock is provided by a crystal Example: clock { diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c index 1ed591a..5db9bf0 100644 --- a/drivers/clk/clk-fixed-rate.c +++ b/drivers/clk/clk-fixed-rate.c @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node) struct clk *clk; const char *clk_name = node->name; u32 rate; + unsigned long flags = CLK_IS_ROOT; if (of_property_read_u32(node, "clock-frequency", &rate)) return; + if (of_property_read_bool(node, "clock-xtal")) + flags |= CLK_IS_TYPE_XTAL; + of_property_read_string(node, "clock-output-names", &clk_name); - clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate); + clk = clk_register_fixed_rate(NULL, clk_name, NULL, flags, rate); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk); } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 73bdb69..30c0c37 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -29,6 +29,7 @@ #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ #define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */ +#define CLK_IS_TYPE_XTAL BIT(8) /* this is a crystal clock */ struct clk_hw; -- 1.8.4.1.559.gdb9bdfb ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-08 18:00 ` [PATCH] " Felipe Balbi @ 2013-11-08 19:16 ` Luca Coelho 2013-11-10 11:37 ` Maxime Ripard 2013-11-11 16:27 ` Stephen Warren 2 siblings, 0 replies; 22+ messages in thread From: Luca Coelho @ 2013-11-08 19:16 UTC (permalink / raw) To: Felipe Balbi Cc: mturquette, Linux ARM Kernel Mailing List, Linux OMAP Mailing List, Linux Kernel Mailing List, james.hogan Hi Felipe, On Fri, 2013-11-08 at 12:00 -0600, Felipe Balbi wrote: > From: Luciano Coelho <luca@coelho.fi> > > Add a flag that indicate whether the clock is a crystal or not. > > Additionally, parse a new device tree binding in clk-fixed-rate to set > this flag. > > If clock-xtal isn't set, the clock framework will assume clock to be > generated by an oscillator. There's only one user for this binding > right now which is Texas Instruments' WiLink devices which need to know > details about the clock in order to initialize the underlying WiFi HW > correctly. > > Signed-off-by: Luciano Coelho <luca@coelho.fi> > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag > isn't there, default behavior will be taken. I don't think this is a good idea, because how would code that relies on the CLK_IS_TYPE_XTAL flag know whether 0 means that the clock is *not* crystal or if the flag is simply not defined? There are many clocks which *are* crystal but don't define this flag (obviously, because it didn't exist). That's the reason why I added the CLK_IS_TYPE_DEFINED flag... If you want to get rid of this flag, you should traverse all clocks and set the CLK_IS_TYPE_XTAL on all those that are crystals. -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-08 18:00 ` [PATCH] " Felipe Balbi 2013-11-08 19:16 ` Luca Coelho @ 2013-11-10 11:37 ` Maxime Ripard 2013-11-11 19:42 ` Felipe Balbi 2013-11-11 16:27 ` Stephen Warren 2 siblings, 1 reply; 22+ messages in thread From: Maxime Ripard @ 2013-11-10 11:37 UTC (permalink / raw) To: Felipe Balbi Cc: mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, luca, Linux ARM Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 2477 bytes --] Hi Felipe, On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote: > From: Luciano Coelho <luca@coelho.fi> > > Add a flag that indicate whether the clock is a crystal or not. > > Additionally, parse a new device tree binding in clk-fixed-rate to set > this flag. > > If clock-xtal isn't set, the clock framework will assume clock to be > generated by an oscillator. There's only one user for this binding > right now which is Texas Instruments' WiLink devices which need to know > details about the clock in order to initialize the underlying WiFi HW > correctly. > > Signed-off-by: Luciano Coelho <luca@coelho.fi> > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag > isn't there, default behavior will be taken. > > Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 + > drivers/clk/clk-fixed-rate.c | 6 +++++- > include/linux/clk-provider.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt > index 0b1fe78..3036dfe 100644 > --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt > +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt > @@ -12,6 +12,7 @@ Required properties: > Optional properties: > - gpios : From common gpio binding; gpio connection to clock enable pin. > - clock-output-names : From common clock binding. > +- clock-xtal: true when a clock is provided by a crystal > > Example: > clock { > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c > index 1ed591a..5db9bf0 100644 > --- a/drivers/clk/clk-fixed-rate.c > +++ b/drivers/clk/clk-fixed-rate.c > @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node) > struct clk *clk; > const char *clk_name = node->name; > u32 rate; > + unsigned long flags = CLK_IS_ROOT; > > if (of_property_read_u32(node, "clock-frequency", &rate)) > return; > > + if (of_property_read_bool(node, "clock-xtal")) > + flags |= CLK_IS_TYPE_XTAL; > + Introducing a new compatible instead of a property would make more sense here I think. Do you have a reason not to do so? Thanks, Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-10 11:37 ` Maxime Ripard @ 2013-11-11 19:42 ` Felipe Balbi 2013-11-11 19:50 ` Luca Coelho 2013-11-11 20:54 ` Maxime Ripard 0 siblings, 2 replies; 22+ messages in thread From: Felipe Balbi @ 2013-11-11 19:42 UTC (permalink / raw) To: Maxime Ripard Cc: Felipe Balbi, mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, luca, Linux ARM Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 2824 bytes --] Hi, On Sun, Nov 10, 2013 at 12:37:16PM +0100, Maxime Ripard wrote: > Hi Felipe, > > On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote: > > From: Luciano Coelho <luca@coelho.fi> > > > > Add a flag that indicate whether the clock is a crystal or not. > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > this flag. > > > > If clock-xtal isn't set, the clock framework will assume clock to be > > generated by an oscillator. There's only one user for this binding > > right now which is Texas Instruments' WiLink devices which need to know > > details about the clock in order to initialize the underlying WiFi HW > > correctly. > > > > Signed-off-by: Luciano Coelho <luca@coelho.fi> > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > --- > > > > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag > > isn't there, default behavior will be taken. > > > > Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 + > > drivers/clk/clk-fixed-rate.c | 6 +++++- > > include/linux/clk-provider.h | 1 + > > 3 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt > > index 0b1fe78..3036dfe 100644 > > --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt > > +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt > > @@ -12,6 +12,7 @@ Required properties: > > Optional properties: > > - gpios : From common gpio binding; gpio connection to clock enable pin. > > - clock-output-names : From common clock binding. > > +- clock-xtal: true when a clock is provided by a crystal > > > > Example: > > clock { > > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c > > index 1ed591a..5db9bf0 100644 > > --- a/drivers/clk/clk-fixed-rate.c > > +++ b/drivers/clk/clk-fixed-rate.c > > @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node) > > struct clk *clk; > > const char *clk_name = node->name; > > u32 rate; > > + unsigned long flags = CLK_IS_ROOT; > > > > if (of_property_read_u32(node, "clock-frequency", &rate)) > > return; > > > > + if (of_property_read_bool(node, "clock-xtal")) > > + flags |= CLK_IS_TYPE_XTAL; > > + > > Introducing a new compatible instead of a property would make more > sense here I think. > > Do you have a reason not to do so? As you can see, this is original work from Luca but I disagree that adding a new compatible makes more sense. This still related to a fixed rate clock, we're just giving it one extra metadata which will differentiate between crystal and oscilator fixed rate clocks. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-11 19:42 ` Felipe Balbi @ 2013-11-11 19:50 ` Luca Coelho 2013-11-11 20:59 ` Maxime Ripard 2013-11-11 20:54 ` Maxime Ripard 1 sibling, 1 reply; 22+ messages in thread From: Luca Coelho @ 2013-11-11 19:50 UTC (permalink / raw) To: balbi Cc: Maxime Ripard, mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, Linux ARM Kernel Mailing List On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote: > Hi, > > On Sun, Nov 10, 2013 at 12:37:16PM +0100, Maxime Ripard wrote: > > Hi Felipe, > > > > On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote: > > > From: Luciano Coelho <luca@coelho.fi> > > > > > > Add a flag that indicate whether the clock is a crystal or not. > > > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > > this flag. > > > > > > If clock-xtal isn't set, the clock framework will assume clock to be > > > generated by an oscillator. There's only one user for this binding > > > right now which is Texas Instruments' WiLink devices which need to know > > > details about the clock in order to initialize the underlying WiFi HW > > > correctly. > > > > > > Signed-off-by: Luciano Coelho <luca@coelho.fi> > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > --- > > > > > > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag > > > isn't there, default behavior will be taken. > > > > > > Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 + > > > drivers/clk/clk-fixed-rate.c | 6 +++++- > > > include/linux/clk-provider.h | 1 + > > > 3 files changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt > > > index 0b1fe78..3036dfe 100644 > > > --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt > > > +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt > > > @@ -12,6 +12,7 @@ Required properties: > > > Optional properties: > > > - gpios : From common gpio binding; gpio connection to clock enable pin. > > > - clock-output-names : From common clock binding. > > > +- clock-xtal: true when a clock is provided by a crystal > > > > > > Example: > > > clock { > > > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c > > > index 1ed591a..5db9bf0 100644 > > > --- a/drivers/clk/clk-fixed-rate.c > > > +++ b/drivers/clk/clk-fixed-rate.c > > > @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node) > > > struct clk *clk; > > > const char *clk_name = node->name; > > > u32 rate; > > > + unsigned long flags = CLK_IS_ROOT; > > > > > > if (of_property_read_u32(node, "clock-frequency", &rate)) > > > return; > > > > > > + if (of_property_read_bool(node, "clock-xtal")) > > > + flags |= CLK_IS_TYPE_XTAL; > > > + > > > > Introducing a new compatible instead of a property would make more > > sense here I think. > > > > Do you have a reason not to do so? > > As you can see, this is original work from Luca but I disagree that > adding a new compatible makes more sense. This still related to a fixed > rate clock, we're just giving it one extra metadata which willAnd t > differentiate between crystal and oscilator fixed rate clocks. I agree with Felipe. This was discussed before [1]. While still at TI, I tried to figure out the exact need for the firmware to know whether it was an oscillator or not. It was mostly because the stabilization time and such things differ with oscillators, but I wasn't able to find out how exactly this affected things. In any case, as I concluded earlier (but it's not really my call), being a crystal or an oscillator *is* a characteristic of the hardware, regardless of whether that information is useful or not. In the WiLink case it is, at least it can differentiate the clocks that are used in the HW modules it uses. So IMHO it doesn't really hurt and it's not really against the DT principles. [1] https://lkml.org/lkml/2013/7/29/321 -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-11 19:50 ` Luca Coelho @ 2013-11-11 20:59 ` Maxime Ripard 2013-11-12 8:05 ` Luca Coelho 0 siblings, 1 reply; 22+ messages in thread From: Maxime Ripard @ 2013-11-11 20:59 UTC (permalink / raw) To: Luca Coelho Cc: balbi, mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, Linux ARM Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1872 bytes --] Hi Luca, On Mon, Nov 11, 2013 at 09:50:56PM +0200, Luca Coelho wrote: > On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote: > > > > + if (of_property_read_bool(node, "clock-xtal")) > > > > + flags |= CLK_IS_TYPE_XTAL; > > > > + > > > > > > Introducing a new compatible instead of a property would make more > > > sense here I think. > > > > > > Do you have a reason not to do so? > > > > As you can see, this is original work from Luca but I disagree that > > adding a new compatible makes more sense. This still related to a fixed > > rate clock, we're just giving it one extra metadata which willAnd t > > differentiate between crystal and oscilator fixed rate clocks. > > I agree with Felipe. This was discussed before [1]. While still at TI, > I tried to figure out the exact need for the firmware to know whether it > was an oscillator or not. It was mostly because the stabilization time > and such things differ with oscillators, but I wasn't able to find out > how exactly this affected things. > > In any case, as I concluded earlier (but it's not really my call), being > a crystal or an oscillator *is* a characteristic of the hardware, > regardless of whether that information is useful or not. In the WiLink > case it is, at least it can differentiate the clocks that are used in > the HW modules it uses. > > So IMHO it doesn't really hurt and it's not really against the DT > principles. Just to be clear, I'm not against your patch. If you need this to make your driver work, then it's fine for me. Mike will probably know better if we actually need some extra metadata. What I'm not really convinced about is *how* you carry that metadata in the DT, that's all, nothing more. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-11 20:59 ` Maxime Ripard @ 2013-11-12 8:05 ` Luca Coelho 2013-11-13 14:40 ` Maxime Ripard 0 siblings, 1 reply; 22+ messages in thread From: Luca Coelho @ 2013-11-12 8:05 UTC (permalink / raw) To: Maxime Ripard Cc: balbi, mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, Linux ARM Kernel Mailing List On Mon, 2013-11-11 at 21:59 +0100, Maxime Ripard wrote: > Hi Luca, > > On Mon, Nov 11, 2013 at 09:50:56PM +0200, Luca Coelho wrote: > > On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote: > > > > > + if (of_property_read_bool(node, "clock-xtal")) > > > > > + flags |= CLK_IS_TYPE_XTAL; > > > > > + > > > > > > > > Introducing a new compatible instead of a property would make more > > > > sense here I think. > > > > > > > > Do you have a reason not to do so? > > > > > > As you can see, this is original work from Luca but I disagree that > > > adding a new compatible makes more sense. This still related to a fixed > > > rate clock, we're just giving it one extra metadata which willAnd t > > > differentiate between crystal and oscilator fixed rate clocks. > > > > I agree with Felipe. This was discussed before [1]. While still at TI, > > I tried to figure out the exact need for the firmware to know whether it > > was an oscillator or not. It was mostly because the stabilization time > > and such things differ with oscillators, but I wasn't able to find out > > how exactly this affected things. > > > > In any case, as I concluded earlier (but it's not really my call), being > > a crystal or an oscillator *is* a characteristic of the hardware, > > regardless of whether that information is useful or not. In the WiLink > > case it is, at least it can differentiate the clocks that are used in > > the HW modules it uses. > > > > So IMHO it doesn't really hurt and it's not really against the DT > > principles. > > Just to be clear, I'm not against your patch. If you need this to make > your driver work, then it's fine for me. Mike will probably know > better if we actually need some extra metadata. :) I understand, we should really try to make this as clean as possible, DT should really be a good description of the hardware. > What I'm not really convinced about is *how* you carry that metadata > in the DT, that's all, nothing more. Okay, I get you. My point is that being a crystal or not *is* a characteristic of the clock, so I think it could be part of the flags that describe it. In any case, it's not really my call. This is about the clock and it's not even my home turf (wireless). ;) Thanks for your comments. And I'm sorry if the tone of my previous email sounded harsh, it was not supposed to. :) -- Cheers, Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-12 8:05 ` Luca Coelho @ 2013-11-13 14:40 ` Maxime Ripard 0 siblings, 0 replies; 22+ messages in thread From: Maxime Ripard @ 2013-11-13 14:40 UTC (permalink / raw) To: Luca Coelho Cc: balbi, mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, Linux ARM Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 900 bytes --] On Tue, Nov 12, 2013 at 10:05:49AM +0200, Luca Coelho wrote: > > What I'm not really convinced about is *how* you carry that metadata > > in the DT, that's all, nothing more. > > Okay, I get you. My point is that being a crystal or not *is* a > characteristic of the clock, so I think it could be part of the flags > that describe it. Yeah, but then, a crystal has a slightly different behaviour and property than a oscillator, therefore the two aren't fully "compatible" with each other. > In any case, it's not really my call. This is about the clock and it's > not even my home turf (wireless). ;) > > Thanks for your comments. And I'm sorry if the tone of my previous > email sounded harsh, it was not supposed to. :) That's fine, don't worry :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-11 19:42 ` Felipe Balbi 2013-11-11 19:50 ` Luca Coelho @ 2013-11-11 20:54 ` Maxime Ripard 1 sibling, 0 replies; 22+ messages in thread From: Maxime Ripard @ 2013-11-11 20:54 UTC (permalink / raw) To: Felipe Balbi Cc: mturquette, james.hogan, Linux Kernel Mailing List, Linux OMAP Mailing List, luca, Linux ARM Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1184 bytes --] On Mon, Nov 11, 2013 at 01:42:47PM -0600, Felipe Balbi wrote: > > > + if (of_property_read_bool(node, "clock-xtal")) > > > + flags |= CLK_IS_TYPE_XTAL; > > > + > > > > Introducing a new compatible instead of a property would make more > > sense here I think. > > > > Do you have a reason not to do so? > > As you can see, this is original work from Luca but I disagree that > adding a new compatible makes more sense. This still related to a fixed > rate clock, we're just giving it one extra metadata which will > differentiate between crystal and oscilator fixed rate clocks. I don't know, I think it's more a matter of consistency. If we turn the problem the other way around. Let's say we have a crystal that for some reason can't be used with clk-fixed-rate. You'd add a new driver for it, with a compatible of its own, and you'd put that XTAL flag in there, without any extra metadata in the DT, right? And I'm pretty sure having a compatible like "clk-xtal" would make it pretty obvious that it's still a fixed rate clock. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-08 18:00 ` [PATCH] " Felipe Balbi 2013-11-08 19:16 ` Luca Coelho 2013-11-10 11:37 ` Maxime Ripard @ 2013-11-11 16:27 ` Stephen Warren 2013-11-11 19:43 ` Felipe Balbi 2 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2013-11-11 16:27 UTC (permalink / raw) To: Felipe Balbi, mturquette Cc: Linux ARM Kernel Mailing List, Linux OMAP Mailing List, Linux Kernel Mailing List, james.hogan, luca On 11/08/2013 11:00 AM, Felipe Balbi wrote: > From: Luciano Coelho <luca@coelho.fi> > > Add a flag that indicate whether the clock is a crystal or not. > > Additionally, parse a new device tree binding in clk-fixed-rate to set > this flag. > > If clock-xtal isn't set, the clock framework will assume clock to be > generated by an oscillator. There's only one user for this binding > right now which is Texas Instruments' WiLink devices which need to know > details about the clock in order to initialize the underlying WiFi HW > correctly. Why on earth does it care? Surely the WiFi HW doesn't care about crystal-vs-non-crystal, but rather some facet of the clock signal that the type of source implies. Shouldn't the DT property describe that facet of the signal, rather than the reason why it has that facet? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] clk: add flags to distinguish xtal clocks 2013-11-11 16:27 ` Stephen Warren @ 2013-11-11 19:43 ` Felipe Balbi 0 siblings, 0 replies; 22+ messages in thread From: Felipe Balbi @ 2013-11-11 19:43 UTC (permalink / raw) To: Stephen Warren Cc: Felipe Balbi, mturquette, Linux ARM Kernel Mailing List, Linux OMAP Mailing List, Linux Kernel Mailing List, james.hogan, luca [-- Attachment #1: Type: text/plain, Size: 1092 bytes --] Hi, On Mon, Nov 11, 2013 at 09:27:58AM -0700, Stephen Warren wrote: > On 11/08/2013 11:00 AM, Felipe Balbi wrote: > > From: Luciano Coelho <luca@coelho.fi> > > > > Add a flag that indicate whether the clock is a crystal or not. > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > this flag. > > > > If clock-xtal isn't set, the clock framework will assume clock to be > > generated by an oscillator. There's only one user for this binding > > right now which is Texas Instruments' WiLink devices which need to know > > details about the clock in order to initialize the underlying WiFi HW > > correctly. > > Why on earth does it care? Surely the WiFi HW doesn't care about > crystal-vs-non-crystal, but rather some facet of the clock signal that > the type of source implies. Shouldn't the DT property describe that > facet of the signal, rather than the reason why it has that facet? well, if you can figure out what that facet is, then _do_ tell. Luca has tried for months to get that information with no success. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-07-04 21:05 [RFC] clk: add flags to distinguish xtal clocks Luciano Coelho [not found] ` <20130704222538.10823.2559@quantum> @ 2013-07-05 13:12 ` James Hogan 2013-07-05 13:21 ` Felipe Balbi 2013-07-05 13:22 ` Luciano Coelho 1 sibling, 2 replies; 22+ messages in thread From: James Hogan @ 2013-07-05 13:12 UTC (permalink / raw) To: Luciano Coelho; +Cc: Mike Turquette, linux-arm-kernel, LKML, balbi On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote: > Add a flag that indicate whether the clock is a crystal or not. Since > no clocks set this flag right now, include an additional flag that > indicates whether the type is set or not. If the CLK_IS_TYPE_DEFINED > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined. > This ensures backwards compatibility. > > Additionally, parse a new device tree binding in clk-fixed-rate to set > this flag. > > Signed-off-by: Luciano Coelho <coelho@ti.com> > --- > > I'm not familiar with the common clock framework and I'm not > entirely sure the flags can be used in such a way, but to me it looks > reasonable, since some clock consumers may need to know what type of > clock is being provided. > > Specifically, the wl12xx firmware needs to know if the clock is XTAL > or not to handle the stabilization and boosts properly. > > My main idea is that I need to pass this information in the device > tree definition of the clocks, so that the driver can pass this > information on to the firmware. > > Please let me know if this looks ok or not. If not, please let me > know if you have any other ideas on how to solve my problem (of > knowing whether the clock attached to the WiLink chip is XTAL or not). The TZ1090 SoC has something that sounds possibly similar, where some of the XTAL pads have a bypass bit, which according to the hardware engineer I asked should be enabled when you want to use the corresponding XTAL pads as a clock input pad rather than an oscillator. I was considering extending clk-fixed-rate (via a wrapper driver) to parse a custom DT property and a register address / bit number and set the bypass bit as appropriate itself. So I was wondering, is there a particular reason you don't have a DT property on the node for the device that needs to know what type of clock it is, rather than the clock node itself? That way you're not depending directly on the generic common clock framework to be able to tell you such electrical details. Cheers James ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-07-05 13:12 ` [RFC] " James Hogan @ 2013-07-05 13:21 ` Felipe Balbi 2013-07-05 13:22 ` Luciano Coelho 1 sibling, 0 replies; 22+ messages in thread From: Felipe Balbi @ 2013-07-05 13:21 UTC (permalink / raw) To: James Hogan; +Cc: Luciano Coelho, Mike Turquette, linux-arm-kernel, LKML, balbi [-- Attachment #1: Type: text/plain, Size: 2882 bytes --] On Fri, Jul 05, 2013 at 02:12:08PM +0100, James Hogan wrote: > On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote: > > Add a flag that indicate whether the clock is a crystal or not. Since > > no clocks set this flag right now, include an additional flag that > > indicates whether the type is set or not. If the CLK_IS_TYPE_DEFINED > > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined. > > This ensures backwards compatibility. > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > this flag. > > > > Signed-off-by: Luciano Coelho <coelho@ti.com> > > --- > > > > I'm not familiar with the common clock framework and I'm not > > entirely sure the flags can be used in such a way, but to me it looks > > reasonable, since some clock consumers may need to know what type of > > clock is being provided. > > > > Specifically, the wl12xx firmware needs to know if the clock is XTAL > > or not to handle the stabilization and boosts properly. > > > > My main idea is that I need to pass this information in the device > > tree definition of the clocks, so that the driver can pass this > > information on to the firmware. > > > > Please let me know if this looks ok or not. If not, please let me > > know if you have any other ideas on how to solve my problem (of > > knowing whether the clock attached to the WiLink chip is XTAL or not). > > The TZ1090 SoC has something that sounds possibly similar, where some > of the XTAL pads have a bypass bit, which according to the hardware > engineer I asked should be enabled when you want to use the > corresponding XTAL pads as a clock input pad rather than an > oscillator. I was considering extending clk-fixed-rate (via a wrapper > driver) to parse a custom DT property and a register address / bit > number and set the bypass bit as appropriate itself. > > So I was wondering, is there a particular reason you don't have a DT > property on the node for the device that needs to know what type of > clock it is, rather than the clock node itself? That way you're not > depending directly on the generic common clock framework to be able to > tell you such electrical details. three things here: 1) you end up with several devices implementing the clock type attribute. 2) the type is a property of the clock itself 3) At least WiLink, can work with both types of clocks. considering those, I really think this should belong to the clock node. Otherwise Imagine how your DT would look like: clock { compatible = "fixed-rate"; ... }; wilink { clocks = &clock; wilink,btw-this-time-we-are-using-xtal; ... }; where you could: clock { compatible = "fixed-rate"; clock-type = "xtal"; ... }; wilink { clocks = &clock; ... }; second option looks a lot cleaner to me. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] clk: add flags to distinguish xtal clocks 2013-07-05 13:12 ` [RFC] " James Hogan 2013-07-05 13:21 ` Felipe Balbi @ 2013-07-05 13:22 ` Luciano Coelho 1 sibling, 0 replies; 22+ messages in thread From: Luciano Coelho @ 2013-07-05 13:22 UTC (permalink / raw) To: James Hogan; +Cc: Mike Turquette, linux-arm-kernel, LKML, balbi On Fri, 2013-07-05 at 14:12 +0100, James Hogan wrote: > On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote: > > Add a flag that indicate whether the clock is a crystal or not. Since > > no clocks set this flag right now, include an additional flag that > > indicates whether the type is set or not. If the CLK_IS_TYPE_DEFINED > > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined. > > This ensures backwards compatibility. > > > > Additionally, parse a new device tree binding in clk-fixed-rate to set > > this flag. > > > > Signed-off-by: Luciano Coelho <coelho@ti.com> > > --- > > > > I'm not familiar with the common clock framework and I'm not > > entirely sure the flags can be used in such a way, but to me it looks > > reasonable, since some clock consumers may need to know what type of > > clock is being provided. > > > > Specifically, the wl12xx firmware needs to know if the clock is XTAL > > or not to handle the stabilization and boosts properly. > > > > My main idea is that I need to pass this information in the device > > tree definition of the clocks, so that the driver can pass this > > information on to the firmware. > > > > Please let me know if this looks ok or not. If not, please let me > > know if you have any other ideas on how to solve my problem (of > > knowing whether the clock attached to the WiLink chip is XTAL or not). > > The TZ1090 SoC has something that sounds possibly similar, where some > of the XTAL pads have a bypass bit, which according to the hardware > engineer I asked should be enabled when you want to use the > corresponding XTAL pads as a clock input pad rather than an > oscillator. Cool, good to know that I'm not alone here. ;) > I was considering extending clk-fixed-rate (via a wrapper > driver) to parse a custom DT property and a register address / bit > number and set the bypass bit as appropriate itself. I thought about this too. I actually have a "driver" for my clocks, because normally they're not part of the main board, but part of the module that contains the WiLink chip. In those cases, I don't want my clocks to be used as a generic clk-fixed-rate by the clock framework. But the only difference in my "wrapper" is that it matches "ti,wilink-clock" instead of "fixed-clock". > So I was wondering, is there a particular reason you don't have a DT > property on the node for the device that needs to know what type of > clock it is, rather than the clock node itself? That way you're not > depending directly on the generic common clock framework to be able to > tell you such electrical details. I think this is a detail of the clock itself, not of the user of the clock. Of course, the user of the clock needs to know what to do if the clock is XTAL. But the information of whether it is a XTAL or not should be in the clock data. I tried to make a generic solution that everyone could use. For instance, you. :) You could use the same bit that I implemented and, in your driver, convert that bit into the action you need to take. -- Luca. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-11-13 14:45 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-04 21:05 [RFC] clk: add flags to distinguish xtal clocks Luciano Coelho [not found] ` <20130704222538.10823.2559@quantum> 2013-07-04 22:37 ` Luciano Coelho [not found] ` <20130704231953.10823.94331@quantum> 2013-07-05 7:54 ` Luciano Coelho 2013-07-29 13:50 ` Luciano Coelho [not found] ` <20131007074424.7445.52119@quantum> 2013-10-08 15:27 ` Felipe Balbi 2013-10-16 10:24 ` Luca Coelho 2013-10-23 9:24 ` Mike Turquette 2013-10-23 11:35 ` Luca Coelho 2013-11-08 18:00 ` [PATCH] " Felipe Balbi 2013-11-08 19:16 ` Luca Coelho 2013-11-10 11:37 ` Maxime Ripard 2013-11-11 19:42 ` Felipe Balbi 2013-11-11 19:50 ` Luca Coelho 2013-11-11 20:59 ` Maxime Ripard 2013-11-12 8:05 ` Luca Coelho 2013-11-13 14:40 ` Maxime Ripard 2013-11-11 20:54 ` Maxime Ripard 2013-11-11 16:27 ` Stephen Warren 2013-11-11 19:43 ` Felipe Balbi 2013-07-05 13:12 ` [RFC] " James Hogan 2013-07-05 13:21 ` Felipe Balbi 2013-07-05 13:22 ` Luciano Coelho
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).