| Message ID | 1372971912-10877-1-git-send-email-coelho@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, 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? 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)? Regards, Mike > > 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
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.
Quoting Luciano Coelho (2013-07-04 15:37:45) > 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. Thanks for the explanation. > > > > 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. 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. Regards, Mike > > -- > Luca.
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.
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
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.
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.
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.
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.
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.
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.
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.
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.
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;
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(-)