| Message ID | 1423097705-22939-2-git-send-email-rjui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/04/15 16:55, Ray Jui wrote: > Sometimes a clock needs to know the rate of its parent before itself is > registered to the framework. An example is that a PLL may need to > initialize itself to a specific VCO frequency, before registering to the > framework. The parent rate needs to be known, for PLL multipliers and > divisors to be configured properly. > > Introduce helper function of_clk_get_parent_rate, which can be used to > obtain the parent rate of a clock, given a device node and index. > > Signed-off-by: Ray Jui <rjui@broadcom.com> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Hi Ray, On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: > Sometimes a clock needs to know the rate of its parent before itself is > registered to the framework. An example is that a PLL may need to > initialize itself to a specific VCO frequency, before registering to the > framework. The parent rate needs to be known, for PLL multipliers and > divisors to be configured properly. > > Introduce helper function of_clk_get_parent_rate, which can be used to > obtain the parent rate of a clock, given a device node and index. I can't see how this patch helps you. First it's not guaranteed that the parent is already registered, what do you do in this case? Then the clock framework doesn't require that you initialize the PLL before registering. That can be done in the clk ops later. Sascha
Hi Sascha, On 2/25/2015 9:54 PM, Sascha Hauer wrote: > Hi Ray, > > On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: >> Sometimes a clock needs to know the rate of its parent before itself is >> registered to the framework. An example is that a PLL may need to >> initialize itself to a specific VCO frequency, before registering to the >> framework. The parent rate needs to be known, for PLL multipliers and >> divisors to be configured properly. >> >> Introduce helper function of_clk_get_parent_rate, which can be used to >> obtain the parent rate of a clock, given a device node and index. > > I can't see how this patch helps you. First it's not guaranteed that > the parent is already registered, what do you do in this case? In the case when clock parent is not found, as you can see from the code, it simply returns zero, just like other clk get rate APIs. I thought the order of clock registration is based on order of the clock nodes in device tree. It makes sense to me to declare the parent clock before a child clock, so it's guaranteed that the parent is registered before the child. > Then the clock framework doesn't require that you initialize the PLL > before registering. That can be done in the clk ops later. Sure it's not mandatory. But what's wrong with me choosing to initialize the PLL clock to a known frequency before registering it to the framework? Ray
On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: > Hi Sascha, > > On 2/25/2015 9:54 PM, Sascha Hauer wrote: > > Hi Ray, > > > > On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: > >> Sometimes a clock needs to know the rate of its parent before itself is > >> registered to the framework. An example is that a PLL may need to > >> initialize itself to a specific VCO frequency, before registering to the > >> framework. The parent rate needs to be known, for PLL multipliers and > >> divisors to be configured properly. > >> > >> Introduce helper function of_clk_get_parent_rate, which can be used to > >> obtain the parent rate of a clock, given a device node and index. > > > > I can't see how this patch helps you. First it's not guaranteed that > > the parent is already registered, what do you do in this case? > > In the case when clock parent is not found, as you can see from the > code, it simply returns zero, just like other clk get rate APIs. Yes, but what do you do with the 0 result then in your PLL initialization? > > I thought the order of clock registration is based on order of the clock > nodes in device tree. It makes sense to me to declare the parent clock > before a child clock, so it's guaranteed that the parent is registered > before the child. No, you can't rely on that. The order of the device nodes may happen to define the order of clock initialization now, but that may change. device nodes are usually ordered by bus addresses, not by intended initialization order. Even if you reorder them everything must still work. > > > Then the clock framework doesn't require that you initialize the PLL > > before registering. That can be done in the clk ops later. > > Sure it's not mandatory. But what's wrong with me choosing to initialize > the PLL clock to a known frequency before registering it to the framework? Appearantly you don't know the (input) frequency of the PLL when registering it to the framework, so the question must be: What's wrong with keeping it uninitialized? If the PLL is unused then you don't care about it's initialization status. If it happens to be enabled by a bootloader and still unused at late_initcall time the clock framework will disable it so you have a known state then. If a consumer for the PLL appears it's its job to initialize it through the clk api. Sascha
On 2/25/2015 10:51 PM, Sascha Hauer wrote: > On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: >> Hi Sascha, >> >> On 2/25/2015 9:54 PM, Sascha Hauer wrote: >>> Hi Ray, >>> >>> On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: >>>> Sometimes a clock needs to know the rate of its parent before itself is >>>> registered to the framework. An example is that a PLL may need to >>>> initialize itself to a specific VCO frequency, before registering to the >>>> framework. The parent rate needs to be known, for PLL multipliers and >>>> divisors to be configured properly. >>>> >>>> Introduce helper function of_clk_get_parent_rate, which can be used to >>>> obtain the parent rate of a clock, given a device node and index. >>> >>> I can't see how this patch helps you. First it's not guaranteed that >>> the parent is already registered, what do you do in this case? >> >> In the case when clock parent is not found, as you can see from the >> code, it simply returns zero, just like other clk get rate APIs. > > Yes, but what do you do with the 0 result then in your PLL initialization? > As of the current code, it fails the PLL frequency initialization and bails out. Thinking about it more, it actually makes more sense to just warn and still go ahead to register the clock, in which case it will use whatever default frequency after chip power on reset or a frequency configured in the bootloader. >> >> I thought the order of clock registration is based on order of the clock >> nodes in device tree. It makes sense to me to declare the parent clock >> before a child clock, so it's guaranteed that the parent is registered >> before the child. > > No, you can't rely on that. The order of the device nodes may happen to > define the order of clock initialization now, but that may change. > device nodes are usually ordered by bus addresses, not by intended > initialization order. Even if you reorder them everything must still > work. > Okay I get your point that the order of device nodes may not be relied on for device initialization order. But then another mechanism should be deployed to give developers the option to decide on the clock initialization sequence. It can be optional but it should be there. >> >>> Then the clock framework doesn't require that you initialize the PLL >>> before registering. That can be done in the clk ops later. >> >> Sure it's not mandatory. But what's wrong with me choosing to initialize >> the PLL clock to a known frequency before registering it to the framework? > > Appearantly you don't know the (input) frequency of the PLL when > registering it to the framework, so the question must be: What's wrong > with keeping it uninitialized? > > If the PLL is unused then you don't care about it's initialization > status. If it happens to be enabled by a bootloader and still unused > at late_initcall time the clock framework will disable it so you > have a known state then. If a consumer for the PLL appears it's its > job to initialize it through the clk api. > > Sascha > Okay, what we need here is to initialize the PLL to a desired frequency, based on device tree settings (since it will be configured differently, among different boards). This is a PLL that 1) has limited options of frequencies which it can be configured to, and 2) has multiple child clocks, where is a more suitable place to initialize it to the desired frequency than right before registering it to the framework? I know a lot of people do it in the bootloader, but I thought we should be given the flexibility of configuring it in the kernel. When you say "consumers", do you mean 1) the device driver that uses the PLL; or 2) the device driver that use the child clock of the PLL? If it's case 1), then we don't really have a device driver that directly uses the PLL, and I thought that's quite normal, as most PLLs don't directly feed into any peripherals. We do have multiple device drivers that use the child clocks of the PLL, but it makes no sense to configure the PLL clock in any of those drivers. Ray
On Wed, Feb 25, 2015 at 11:42:44PM -0800, Ray Jui wrote: > On 2/25/2015 10:51 PM, Sascha Hauer wrote: > > On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: > >> Hi Sascha, > >> > >> On 2/25/2015 9:54 PM, Sascha Hauer wrote: > >>> Hi Ray, > >>> > >>> On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: > >>>> Sometimes a clock needs to know the rate of its parent before itself is > >>>> registered to the framework. An example is that a PLL may need to > >>>> initialize itself to a specific VCO frequency, before registering to the > >>>> framework. The parent rate needs to be known, for PLL multipliers and > >>>> divisors to be configured properly. > >>>> > >>>> Introduce helper function of_clk_get_parent_rate, which can be used to > >>>> obtain the parent rate of a clock, given a device node and index. > >>> > >>> I can't see how this patch helps you. First it's not guaranteed that > >>> the parent is already registered, what do you do in this case? > >> > >> In the case when clock parent is not found, as you can see from the > >> code, it simply returns zero, just like other clk get rate APIs. > > > > Yes, but what do you do with the 0 result then in your PLL initialization? > > > > As of the current code, it fails the PLL frequency initialization and > bails out. Thinking about it more, it actually makes more sense to just > warn and still go ahead to register the clock, in which case it will use > whatever default frequency after chip power on reset or a frequency > configured in the bootloader. > > >> > >> I thought the order of clock registration is based on order of the clock > >> nodes in device tree. It makes sense to me to declare the parent clock > >> before a child clock, so it's guaranteed that the parent is registered > >> before the child. > > > > No, you can't rely on that. The order of the device nodes may happen to > > define the order of clock initialization now, but that may change. > > device nodes are usually ordered by bus addresses, not by intended > > initialization order. Even if you reorder them everything must still > > work. > > > > Okay I get your point that the order of device nodes may not be relied > on for device initialization order. But then another mechanism should be > deployed to give developers the option to decide on the clock > initialization sequence. It can be optional but it should be there. > > >> > >>> Then the clock framework doesn't require that you initialize the PLL > >>> before registering. That can be done in the clk ops later. > >> > >> Sure it's not mandatory. But what's wrong with me choosing to initialize > >> the PLL clock to a known frequency before registering it to the framework? > > > > Appearantly you don't know the (input) frequency of the PLL when > > registering it to the framework, so the question must be: What's wrong > > with keeping it uninitialized? > > > > If the PLL is unused then you don't care about it's initialization > > status. If it happens to be enabled by a bootloader and still unused > > at late_initcall time the clock framework will disable it so you > > have a known state then. If a consumer for the PLL appears it's its > > job to initialize it through the clk api. > > > > Sascha > > > > Okay, what we need here is to initialize the PLL to a desired frequency, > based on device tree settings (since it will be configured differently, > among different boards). This is a PLL that 1) has limited options of > frequencies which it can be configured to, and 2) has multiple child > clocks, where is a more suitable place to initialize it to the desired > frequency than right before registering it to the framework? I know a > lot of people do it in the bootloader, but I thought we should be given > the flexibility of configuring it in the kernel. > > When you say "consumers", do you mean 1) the device driver that uses the > PLL; or 2) the device driver that use the child clock of the PLL? If > it's case 1), then we don't really have a device driver that directly > uses the PLL, and I thought that's quite normal, as most PLLs don't > directly feed into any peripherals. I meant 1) and 2). Before a consumer comes along the state of the PLL doesn't matter. When a consumer shows up it has to call clk_prepare_enable which (directly or indirectly) will enable your PLL. Then it's still time to apply the default settings you found out during probe of the PLL. Sascha
Quoting Sascha Hauer (2015-02-26 00:43:19) > On Wed, Feb 25, 2015 at 11:42:44PM -0800, Ray Jui wrote: > > On 2/25/2015 10:51 PM, Sascha Hauer wrote: > > > On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: > > >> Hi Sascha, > > >> > > >> On 2/25/2015 9:54 PM, Sascha Hauer wrote: > > >>> Hi Ray, > > >>> > > >>> On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: > > >>>> Sometimes a clock needs to know the rate of its parent before itself is > > >>>> registered to the framework. An example is that a PLL may need to > > >>>> initialize itself to a specific VCO frequency, before registering to the > > >>>> framework. The parent rate needs to be known, for PLL multipliers and > > >>>> divisors to be configured properly. > > >>>> > > >>>> Introduce helper function of_clk_get_parent_rate, which can be used to > > >>>> obtain the parent rate of a clock, given a device node and index. > > >>> > > >>> I can't see how this patch helps you. First it's not guaranteed that > > >>> the parent is already registered, what do you do in this case? > > >> > > >> In the case when clock parent is not found, as you can see from the > > >> code, it simply returns zero, just like other clk get rate APIs. > > > > > > Yes, but what do you do with the 0 result then in your PLL initialization? > > > > > > > As of the current code, it fails the PLL frequency initialization and > > bails out. Thinking about it more, it actually makes more sense to just > > warn and still go ahead to register the clock, in which case it will use > > whatever default frequency after chip power on reset or a frequency > > configured in the bootloader. > > > > >> > > >> I thought the order of clock registration is based on order of the clock > > >> nodes in device tree. It makes sense to me to declare the parent clock > > >> before a child clock, so it's guaranteed that the parent is registered > > >> before the child. > > > > > > No, you can't rely on that. The order of the device nodes may happen to > > > define the order of clock initialization now, but that may change. > > > device nodes are usually ordered by bus addresses, not by intended > > > initialization order. Even if you reorder them everything must still > > > work. > > > > > > > Okay I get your point that the order of device nodes may not be relied > > on for device initialization order. But then another mechanism should be > > deployed to give developers the option to decide on the clock > > initialization sequence. It can be optional but it should be there. > > > > >> > > >>> Then the clock framework doesn't require that you initialize the PLL > > >>> before registering. That can be done in the clk ops later. > > >> > > >> Sure it's not mandatory. But what's wrong with me choosing to initialize > > >> the PLL clock to a known frequency before registering it to the framework? > > > > > > Appearantly you don't know the (input) frequency of the PLL when > > > registering it to the framework, so the question must be: What's wrong > > > with keeping it uninitialized? > > > > > > If the PLL is unused then you don't care about it's initialization > > > status. If it happens to be enabled by a bootloader and still unused > > > at late_initcall time the clock framework will disable it so you > > > have a known state then. If a consumer for the PLL appears it's its > > > job to initialize it through the clk api. > > > > > > Sascha > > > > > > > Okay, what we need here is to initialize the PLL to a desired frequency, > > based on device tree settings (since it will be configured differently, > > among different boards). This is a PLL that 1) has limited options of > > frequencies which it can be configured to, and 2) has multiple child > > clocks, where is a more suitable place to initialize it to the desired > > frequency than right before registering it to the framework? I know a > > lot of people do it in the bootloader, but I thought we should be given > > the flexibility of configuring it in the kernel. > > > > When you say "consumers", do you mean 1) the device driver that uses the > > PLL; or 2) the device driver that use the child clock of the PLL? If > > it's case 1), then we don't really have a device driver that directly > > uses the PLL, and I thought that's quite normal, as most PLLs don't > > directly feed into any peripherals. > > I meant 1) and 2). Before a consumer comes along the state of the PLL > doesn't matter. When a consumer shows up it has to call > clk_prepare_enable which (directly or indirectly) will enable your PLL. > Then it's still time to apply the default settings you found out during > probe of the PLL. My review comments are really for iproc_pll_setup() in patch #3, but the discussion is here so I'll respond to this thread. I think the root of this problem is that your pll clk_ops does not support .set_rate. That is why your clock driver hacks in a call to pll_set_rate in iproc_pll_setup. Due to the above shortcoming you also do not use the assigned-clock-rate infrastructure to set your pll rate at registration-time. There is no reason for your driver to re-invent this logic. iproc_pll_setup is fetching the clock-frequency property from DT and then trying to set that rate. Instead please use the generic code. The right way to handle this is to support a .set_rate callback (looks like you're 90% of the way there with pll_set_rate) and then use the assigned-clock-rates property to specify this from DT. Regards, Mike > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Mike, On 3/6/2015 11:55 AM, Mike Turquette wrote: > Quoting Sascha Hauer (2015-02-26 00:43:19) >> On Wed, Feb 25, 2015 at 11:42:44PM -0800, Ray Jui wrote: >>> On 2/25/2015 10:51 PM, Sascha Hauer wrote: >>>> On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: >>>>> Hi Sascha, >>>>> >>>>> On 2/25/2015 9:54 PM, Sascha Hauer wrote: >>>>>> Hi Ray, >>>>>> >>>>>> On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: >>>>>>> Sometimes a clock needs to know the rate of its parent before itself is >>>>>>> registered to the framework. An example is that a PLL may need to >>>>>>> initialize itself to a specific VCO frequency, before registering to the >>>>>>> framework. The parent rate needs to be known, for PLL multipliers and >>>>>>> divisors to be configured properly. >>>>>>> >>>>>>> Introduce helper function of_clk_get_parent_rate, which can be used to >>>>>>> obtain the parent rate of a clock, given a device node and index. >>>>>> >>>>>> I can't see how this patch helps you. First it's not guaranteed that >>>>>> the parent is already registered, what do you do in this case? >>>>> >>>>> In the case when clock parent is not found, as you can see from the >>>>> code, it simply returns zero, just like other clk get rate APIs. >>>> >>>> Yes, but what do you do with the 0 result then in your PLL initialization? >>>> >>> >>> As of the current code, it fails the PLL frequency initialization and >>> bails out. Thinking about it more, it actually makes more sense to just >>> warn and still go ahead to register the clock, in which case it will use >>> whatever default frequency after chip power on reset or a frequency >>> configured in the bootloader. >>> >>>>> >>>>> I thought the order of clock registration is based on order of the clock >>>>> nodes in device tree. It makes sense to me to declare the parent clock >>>>> before a child clock, so it's guaranteed that the parent is registered >>>>> before the child. >>>> >>>> No, you can't rely on that. The order of the device nodes may happen to >>>> define the order of clock initialization now, but that may change. >>>> device nodes are usually ordered by bus addresses, not by intended >>>> initialization order. Even if you reorder them everything must still >>>> work. >>>> >>> >>> Okay I get your point that the order of device nodes may not be relied >>> on for device initialization order. But then another mechanism should be >>> deployed to give developers the option to decide on the clock >>> initialization sequence. It can be optional but it should be there. >>> >>>>> >>>>>> Then the clock framework doesn't require that you initialize the PLL >>>>>> before registering. That can be done in the clk ops later. >>>>> >>>>> Sure it's not mandatory. But what's wrong with me choosing to initialize >>>>> the PLL clock to a known frequency before registering it to the framework? >>>> >>>> Appearantly you don't know the (input) frequency of the PLL when >>>> registering it to the framework, so the question must be: What's wrong >>>> with keeping it uninitialized? >>>> >>>> If the PLL is unused then you don't care about it's initialization >>>> status. If it happens to be enabled by a bootloader and still unused >>>> at late_initcall time the clock framework will disable it so you >>>> have a known state then. If a consumer for the PLL appears it's its >>>> job to initialize it through the clk api. >>>> >>>> Sascha >>>> >>> >>> Okay, what we need here is to initialize the PLL to a desired frequency, >>> based on device tree settings (since it will be configured differently, >>> among different boards). This is a PLL that 1) has limited options of >>> frequencies which it can be configured to, and 2) has multiple child >>> clocks, where is a more suitable place to initialize it to the desired >>> frequency than right before registering it to the framework? I know a >>> lot of people do it in the bootloader, but I thought we should be given >>> the flexibility of configuring it in the kernel. >>> >>> When you say "consumers", do you mean 1) the device driver that uses the >>> PLL; or 2) the device driver that use the child clock of the PLL? If >>> it's case 1), then we don't really have a device driver that directly >>> uses the PLL, and I thought that's quite normal, as most PLLs don't >>> directly feed into any peripherals. >> >> I meant 1) and 2). Before a consumer comes along the state of the PLL >> doesn't matter. When a consumer shows up it has to call >> clk_prepare_enable which (directly or indirectly) will enable your PLL. >> Then it's still time to apply the default settings you found out during >> probe of the PLL. > > My review comments are really for iproc_pll_setup() in patch #3, but the > discussion is here so I'll respond to this thread. > > I think the root of this problem is that your pll clk_ops does not > support .set_rate. That is why your clock driver hacks in a call to > pll_set_rate in iproc_pll_setup. > > Due to the above shortcoming you also do not use the assigned-clock-rate > infrastructure to set your pll rate at registration-time. There is no > reason for your driver to re-invent this logic. iproc_pll_setup is > fetching the clock-frequency property from DT and then trying to set > that rate. Instead please use the generic code. > > The right way to handle this is to support a .set_rate callback (looks > like you're 90% of the way there with pll_set_rate) and then use the > assigned-clock-rates property to specify this from DT. > > Regards, > Mike > Okay. It's good to know that "assigned-clock-rate" can be used and serve exactly what we need here. I'll update my patch series to use that instead. In this case, do you think I should still keep of_clk_get_parent_rate in the patch series? Thanks, Ray >> >> Sascha >> >> -- >> Pengutronix e.K. | | >> Industrial Linux Solutions | http://www.pengutronix.de/ | >> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoting Ray Jui (2015-03-06 12:07:13) > Hi Mike, > > On 3/6/2015 11:55 AM, Mike Turquette wrote: > > Quoting Sascha Hauer (2015-02-26 00:43:19) > >> On Wed, Feb 25, 2015 at 11:42:44PM -0800, Ray Jui wrote: > >>> On 2/25/2015 10:51 PM, Sascha Hauer wrote: > >>>> On Wed, Feb 25, 2015 at 10:13:15PM -0800, Ray Jui wrote: > >>>>> Hi Sascha, > >>>>> > >>>>> On 2/25/2015 9:54 PM, Sascha Hauer wrote: > >>>>>> Hi Ray, > >>>>>> > >>>>>> On Wed, Feb 04, 2015 at 04:55:00PM -0800, Ray Jui wrote: > >>>>>>> Sometimes a clock needs to know the rate of its parent before itself is > >>>>>>> registered to the framework. An example is that a PLL may need to > >>>>>>> initialize itself to a specific VCO frequency, before registering to the > >>>>>>> framework. The parent rate needs to be known, for PLL multipliers and > >>>>>>> divisors to be configured properly. > >>>>>>> > >>>>>>> Introduce helper function of_clk_get_parent_rate, which can be used to > >>>>>>> obtain the parent rate of a clock, given a device node and index. > >>>>>> > >>>>>> I can't see how this patch helps you. First it's not guaranteed that > >>>>>> the parent is already registered, what do you do in this case? > >>>>> > >>>>> In the case when clock parent is not found, as you can see from the > >>>>> code, it simply returns zero, just like other clk get rate APIs. > >>>> > >>>> Yes, but what do you do with the 0 result then in your PLL initialization? > >>>> > >>> > >>> As of the current code, it fails the PLL frequency initialization and > >>> bails out. Thinking about it more, it actually makes more sense to just > >>> warn and still go ahead to register the clock, in which case it will use > >>> whatever default frequency after chip power on reset or a frequency > >>> configured in the bootloader. > >>> > >>>>> > >>>>> I thought the order of clock registration is based on order of the clock > >>>>> nodes in device tree. It makes sense to me to declare the parent clock > >>>>> before a child clock, so it's guaranteed that the parent is registered > >>>>> before the child. > >>>> > >>>> No, you can't rely on that. The order of the device nodes may happen to > >>>> define the order of clock initialization now, but that may change. > >>>> device nodes are usually ordered by bus addresses, not by intended > >>>> initialization order. Even if you reorder them everything must still > >>>> work. > >>>> > >>> > >>> Okay I get your point that the order of device nodes may not be relied > >>> on for device initialization order. But then another mechanism should be > >>> deployed to give developers the option to decide on the clock > >>> initialization sequence. It can be optional but it should be there. > >>> > >>>>> > >>>>>> Then the clock framework doesn't require that you initialize the PLL > >>>>>> before registering. That can be done in the clk ops later. > >>>>> > >>>>> Sure it's not mandatory. But what's wrong with me choosing to initialize > >>>>> the PLL clock to a known frequency before registering it to the framework? > >>>> > >>>> Appearantly you don't know the (input) frequency of the PLL when > >>>> registering it to the framework, so the question must be: What's wrong > >>>> with keeping it uninitialized? > >>>> > >>>> If the PLL is unused then you don't care about it's initialization > >>>> status. If it happens to be enabled by a bootloader and still unused > >>>> at late_initcall time the clock framework will disable it so you > >>>> have a known state then. If a consumer for the PLL appears it's its > >>>> job to initialize it through the clk api. > >>>> > >>>> Sascha > >>>> > >>> > >>> Okay, what we need here is to initialize the PLL to a desired frequency, > >>> based on device tree settings (since it will be configured differently, > >>> among different boards). This is a PLL that 1) has limited options of > >>> frequencies which it can be configured to, and 2) has multiple child > >>> clocks, where is a more suitable place to initialize it to the desired > >>> frequency than right before registering it to the framework? I know a > >>> lot of people do it in the bootloader, but I thought we should be given > >>> the flexibility of configuring it in the kernel. > >>> > >>> When you say "consumers", do you mean 1) the device driver that uses the > >>> PLL; or 2) the device driver that use the child clock of the PLL? If > >>> it's case 1), then we don't really have a device driver that directly > >>> uses the PLL, and I thought that's quite normal, as most PLLs don't > >>> directly feed into any peripherals. > >> > >> I meant 1) and 2). Before a consumer comes along the state of the PLL > >> doesn't matter. When a consumer shows up it has to call > >> clk_prepare_enable which (directly or indirectly) will enable your PLL. > >> Then it's still time to apply the default settings you found out during > >> probe of the PLL. > > > > My review comments are really for iproc_pll_setup() in patch #3, but the > > discussion is here so I'll respond to this thread. > > > > I think the root of this problem is that your pll clk_ops does not > > support .set_rate. That is why your clock driver hacks in a call to > > pll_set_rate in iproc_pll_setup. > > > > Due to the above shortcoming you also do not use the assigned-clock-rate > > infrastructure to set your pll rate at registration-time. There is no > > reason for your driver to re-invent this logic. iproc_pll_setup is > > fetching the clock-frequency property from DT and then trying to set > > that rate. Instead please use the generic code. > > > > The right way to handle this is to support a .set_rate callback (looks > > like you're 90% of the way there with pll_set_rate) and then use the > > assigned-clock-rates property to specify this from DT. > > > > Regards, > > Mike > > > > Okay. It's good to know that "assigned-clock-rate" can be used and serve > exactly what we need here. I'll update my patch series to use that instead. > > In this case, do you think I should still keep of_clk_get_parent_rate in > the patch series? No. Without a user we should drop it, and I also do not like its use of clk_lookup. Thanks, Mike > > Thanks, > > Ray > > >> > >> Sascha > >> > >> -- > >> Pengutronix e.K. | | > >> Industrial Linux Solutions | http://www.pengutronix.de/ | > >> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index d48ac71..e1893a2 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2581,6 +2581,23 @@ const char *of_clk_get_parent_name(struct device_node *np, int index) } EXPORT_SYMBOL_GPL(of_clk_get_parent_name); +unsigned long of_clk_get_parent_rate(struct device_node *np, int index) +{ + const char *parent_name; + struct clk *parent_clk; + + parent_name = of_clk_get_parent_name(np, index); + if (!parent_name) + return 0; + + parent_clk = __clk_lookup(parent_name); + if (!parent_clk) + return 0; + + return clk_get_rate(parent_clk); +} +EXPORT_SYMBOL_GPL(of_clk_get_parent_rate); + struct clock_provider { of_clk_init_cb_t clk_init_cb; struct device_node *np; diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index d936409..e1e2d95 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -585,7 +585,7 @@ struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data); int of_clk_get_parent_count(struct device_node *np); const char *of_clk_get_parent_name(struct device_node *np, int index); - +unsigned long of_clk_get_parent_rate(struct device_node *np, int index); void of_clk_init(const struct of_device_id *matches); #else /* !CONFIG_OF */ @@ -614,6 +614,10 @@ static inline const char *of_clk_get_parent_name(struct device_node *np, { return NULL; } +static unsigned long of_clk_get_parent_rate(struct device_node *np, int index) +{ + return 0; +} #define of_clk_init(matches) \ { while (0); } #endif /* CONFIG_OF */
Sometimes a clock needs to know the rate of its parent before itself is registered to the framework. An example is that a PLL may need to initialize itself to a specific VCO frequency, before registering to the framework. The parent rate needs to be known, for PLL multipliers and divisors to be configured properly. Introduce helper function of_clk_get_parent_rate, which can be used to obtain the parent rate of a clock, given a device node and index. Signed-off-by: Ray Jui <rjui@broadcom.com> --- drivers/clk/clk.c | 17 +++++++++++++++++ include/linux/clk-provider.h | 6 +++++- 2 files changed, 22 insertions(+), 1 deletion(-)