diff mbox

[RFC,1/3] clk: inherit display clocks enabled by bootloader

Message ID 20170711182008.28298-2-robdclark@gmail.com (mailing list archive)
State RFC
Delegated to: Stephen Boyd
Headers show

Commit Message

Rob Clark July 11, 2017, 6:20 p.m. UTC
The goal here is to support inheriting a display setup by bootloader,
although there may also be some non-display related use-cases.

Rough idea is to add a flag for clks and power domains that might
already be enabled when kernel starts, and make corresponding fixups
to clk enable/prepare_count and power-domain state so that these are
not automatically disabled late in boot.

If bootloader is enabling display, and kernel is using efifb before
real display driver is loaded (potentially from kernel module after
userspace starts, in a typical distro kernel), we don't want to kill
the clocks and power domains that are used by the display before
userspace starts.

Second part is for drm/msm to check if display related clocks are
enabled when it is loaded, and if so read back hw state to sync
existing display state w/ software state, and skip the initial
clk_enable's and otherwise fixing up clk/regulator/etc ref counts
(skipping the normal display-enable codepaths), therefore inheriting
the enable done by bootloader.

Obviously this should be split up into multiple patches and many
TODOs addressed.  But I guess this is enough for now to start
discussing the approach, and in particular how drm and clock/pd
drivers work together to handle handover from bootloader.

The CLK_INHERIT_BOOTLOADER and related gsdc flag should only be set
on leaf nodes.
---
 drivers/clk/clk.c              | 18 ++++++++++++++++++
 drivers/clk/qcom/common.c      | 28 ++++++++++++++++++++++++++++
 drivers/clk/qcom/gcc-msm8916.c | 15 ++++++++-------
 drivers/clk/qcom/gdsc.c        |  6 ++++++
 drivers/clk/qcom/gdsc.h        |  1 +
 include/linux/clk-provider.h   |  1 +
 include/linux/clk.h            |  9 +++++++++
 7 files changed, 71 insertions(+), 7 deletions(-)

Comments

Rajendra Nayak July 14, 2017, 4:52 a.m. UTC | #1
Hi Rob,

On 07/11/2017 11:50 PM, Rob Clark wrote:
> The goal here is to support inheriting a display setup by bootloader,
> although there may also be some non-display related use-cases.
> 
> Rough idea is to add a flag for clks and power domains that might
> already be enabled when kernel starts, and make corresponding fixups
> to clk enable/prepare_count and power-domain state so that these are
> not automatically disabled late in boot.
> 
> If bootloader is enabling display, and kernel is using efifb before
> real display driver is loaded (potentially from kernel module after
> userspace starts, in a typical distro kernel), we don't want to kill
> the clocks and power domains that are used by the display before
> userspace starts.
> 
> Second part is for drm/msm to check if display related clocks are
> enabled when it is loaded, and if so read back hw state to sync
> existing display state w/ software state, and skip the initial
> clk_enable's and otherwise fixing up clk/regulator/etc ref counts
> (skipping the normal display-enable codepaths), therefore inheriting
> the enable done by bootloader.
> 
> Obviously this should be split up into multiple patches and many
> TODOs addressed.  But I guess this is enough for now to start
> discussing the approach, and in particular how drm and clock/pd
> drivers work together to handle handover from bootloader.
> 
> The CLK_INHERIT_BOOTLOADER and related gsdc flag should only be set
> on leaf nodes.
> ---

[]..

> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index d523991c945f..90b698c910d0 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -11,6 +11,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -258,6 +259,33 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> +	/* Check which of clocks that we inherit state from bootloader
> +	 * are enabled, and fixup enable/prepare state (as well as that
> +	 * of it's parents).
> +	 *
> +	 * TODO can we assume that parents coming from another clk
> +	 * driver are already registered?
> +	 */
> +	for (i = 0; i < num_clks; i++) {
> +		struct clk_hw *hw;
> +
> +		if (!rclks[i])
> +			continue;
> +
> +		hw = &rclks[i]->hw;
> +
> +		if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
> +			continue;
> +
> +		if (!clk_is_enabled_regmap(hw))
> +			continue;
> +
> +		dev_dbg(dev, "%s is enabled from bootloader!\n",
> +			  hw->init->name);
> +
> +		clk_inherit_enabled(hw->clk);

how about just calling a clk_prepare_enable(hw->clk) instead of adding a new API?
The flag could also be something qcom specific and then we avoid having to add
anything in generic CCF code and its all handled in the qcom clock drivers.

> +	}
> +
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
>  	reset->rcdev.ops = &qcom_reset_ops;

[] ..

> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a4f3580587b7..440d819b2d9d 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -291,6 +291,12 @@ static int gdsc_init(struct gdsc *sc)
>  	if ((sc->flags & VOTABLE) && on)
>  		gdsc_enable(&sc->pd);
>  
> +	if ((sc->flags & INHERIT_BL) && on) {
> +		pr_debug("gdsc: %s is enabled from bootloader!\n", sc->pd.name);
> +		gdsc_enable(&sc->pd);
> +		sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;

Would this not prevent the powerdomain from ever getting disabled?

regards,
Rajendra

> +	}
> +
>  	if (on || (sc->pwrsts & PWRSTS_RET))
>  		gdsc_force_mem_on(sc);
>  	else
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 39648348e5ec..3b5e64b060c2 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -53,6 +53,7 @@ struct gdsc {
>  #define VOTABLE		BIT(0)
>  #define CLAMP_IO	BIT(1)
>  #define HW_CTRL		BIT(2)
> +#define INHERIT_BL	BIT(3)
>  	struct reset_controller_dev	*rcdev;
>  	unsigned int			*resets;
>  	unsigned int			reset_count;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index c59c62571e4f..4d5505f92329 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -35,6 +35,7 @@
>  #define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
>  /* parents need enable during gate/ungate, set rate and re-parent */
>  #define CLK_OPS_PARENT_ENABLE	BIT(12)
> +#define CLK_INHERIT_BOOTLOADER	BIT(13) /* clk may be enabled from bootloader */
>  
>  struct clk;
>  struct clk_hw;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 91bd464f4c9b..461991fc57e2 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -391,6 +391,15 @@ void clk_disable(struct clk *clk);
>  void clk_bulk_disable(int num_clks, const struct clk_bulk_data *clks);
>  
>  /**
> + * clk_inherit_enabled - update the enable/prepare count of a clock and it's
> + * parents for clock enabled by bootloader.
> + *
> + * Intended to be used by clock drivers to inform the clk core of a clock
> + * that is already running.
> + */
> +void clk_inherit_enabled(struct clk *clk);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
>   * @clk: clock source
>
Rob Clark July 14, 2017, 10:43 a.m. UTC | #2
On Fri, Jul 14, 2017 at 12:52 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> Hi Rob,
>
> On 07/11/2017 11:50 PM, Rob Clark wrote:
>> The goal here is to support inheriting a display setup by bootloader,
>> although there may also be some non-display related use-cases.
>>
>> Rough idea is to add a flag for clks and power domains that might
>> already be enabled when kernel starts, and make corresponding fixups
>> to clk enable/prepare_count and power-domain state so that these are
>> not automatically disabled late in boot.
>>
>> If bootloader is enabling display, and kernel is using efifb before
>> real display driver is loaded (potentially from kernel module after
>> userspace starts, in a typical distro kernel), we don't want to kill
>> the clocks and power domains that are used by the display before
>> userspace starts.
>>
>> Second part is for drm/msm to check if display related clocks are
>> enabled when it is loaded, and if so read back hw state to sync
>> existing display state w/ software state, and skip the initial
>> clk_enable's and otherwise fixing up clk/regulator/etc ref counts
>> (skipping the normal display-enable codepaths), therefore inheriting
>> the enable done by bootloader.
>>
>> Obviously this should be split up into multiple patches and many
>> TODOs addressed.  But I guess this is enough for now to start
>> discussing the approach, and in particular how drm and clock/pd
>> drivers work together to handle handover from bootloader.
>>
>> The CLK_INHERIT_BOOTLOADER and related gsdc flag should only be set
>> on leaf nodes.
>> ---
>
> []..
>
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index d523991c945f..90b698c910d0 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -11,6 +11,7 @@
>>   * GNU General Public License for more details.
>>   */
>>
>> +#include <linux/clk.h>
>>  #include <linux/export.h>
>>  #include <linux/module.h>
>>  #include <linux/regmap.h>
>> @@ -258,6 +259,33 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>>       if (ret)
>>               return ret;
>>
>> +     /* Check which of clocks that we inherit state from bootloader
>> +      * are enabled, and fixup enable/prepare state (as well as that
>> +      * of it's parents).
>> +      *
>> +      * TODO can we assume that parents coming from another clk
>> +      * driver are already registered?
>> +      */
>> +     for (i = 0; i < num_clks; i++) {
>> +             struct clk_hw *hw;
>> +
>> +             if (!rclks[i])
>> +                     continue;
>> +
>> +             hw = &rclks[i]->hw;
>> +
>> +             if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
>> +                     continue;
>> +
>> +             if (!clk_is_enabled_regmap(hw))
>> +                     continue;
>> +
>> +             dev_dbg(dev, "%s is enabled from bootloader!\n",
>> +                       hw->init->name);
>> +
>> +             clk_inherit_enabled(hw->clk);
>
> how about just calling a clk_prepare_enable(hw->clk) instead of adding a new API?
> The flag could also be something qcom specific and then we avoid having to add
> anything in generic CCF code and its all handled in the qcom clock drivers.

Hmm, I could try that..  I *guess* it doesn't hurt to enable an
already enabled clk..

beyond that, I wonder if this is something that other platforms would
want, so maybe I should be doing the check for enabled in CCF core?
If not, making this a qcom specific flag makes sense.

>> +     }
>> +
>>       reset = &cc->reset;
>>       reset->rcdev.of_node = dev->of_node;
>>       reset->rcdev.ops = &qcom_reset_ops;
>
> [] ..
>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index a4f3580587b7..440d819b2d9d 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -291,6 +291,12 @@ static int gdsc_init(struct gdsc *sc)
>>       if ((sc->flags & VOTABLE) && on)
>>               gdsc_enable(&sc->pd);
>>
>> +     if ((sc->flags & INHERIT_BL) && on) {
>> +             pr_debug("gdsc: %s is enabled from bootloader!\n", sc->pd.name);
>> +             gdsc_enable(&sc->pd);
>> +             sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
>
> Would this not prevent the powerdomain from ever getting disabled?

Yeah, this is a hack, because I couldn't figure out something better.
The problem is that gdsc/powerdomain doesn't have it's own
enable_count but this is tracked at the device level (afaict.. maybe
I'm miss-understanding something).  I guess we could add our own
enable_count within gdsc?  Suggestions welcome, since I don't know
these parts of the code so well.

BR,
-R

> regards,
> Rajendra
>
>> +     }
>> +
>>       if (on || (sc->pwrsts & PWRSTS_RET))
>>               gdsc_force_mem_on(sc);
>>       else
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index 39648348e5ec..3b5e64b060c2 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -53,6 +53,7 @@ struct gdsc {
>>  #define VOTABLE              BIT(0)
>>  #define CLAMP_IO     BIT(1)
>>  #define HW_CTRL              BIT(2)
>> +#define INHERIT_BL   BIT(3)
>>       struct reset_controller_dev     *rcdev;
>>       unsigned int                    *resets;
>>       unsigned int                    reset_count;
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index c59c62571e4f..4d5505f92329 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -35,6 +35,7 @@
>>  #define CLK_IS_CRITICAL              BIT(11) /* do not gate, ever */
>>  /* parents need enable during gate/ungate, set rate and re-parent */
>>  #define CLK_OPS_PARENT_ENABLE        BIT(12)
>> +#define CLK_INHERIT_BOOTLOADER       BIT(13) /* clk may be enabled from bootloader */
>>
>>  struct clk;
>>  struct clk_hw;
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 91bd464f4c9b..461991fc57e2 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -391,6 +391,15 @@ void clk_disable(struct clk *clk);
>>  void clk_bulk_disable(int num_clks, const struct clk_bulk_data *clks);
>>
>>  /**
>> + * clk_inherit_enabled - update the enable/prepare count of a clock and it's
>> + * parents for clock enabled by bootloader.
>> + *
>> + * Intended to be used by clock drivers to inform the clk core of a clock
>> + * that is already running.
>> + */
>> +void clk_inherit_enabled(struct clk *clk);
>> +
>> +/**
>>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>>   *             This is only valid once the clock source has been enabled.
>>   * @clk: clock source
>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajendra Nayak July 17, 2017, 9:37 a.m. UTC | #3
On 7/14/2017 4:13 PM, Rob Clark wrote:
> On Fri, Jul 14, 2017 at 12:52 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> Hi Rob,
>>
>> On 07/11/2017 11:50 PM, Rob Clark wrote:
>>> The goal here is to support inheriting a display setup by bootloader,
>>> although there may also be some non-display related use-cases.
>>>
>>> Rough idea is to add a flag for clks and power domains that might
>>> already be enabled when kernel starts, and make corresponding fixups
>>> to clk enable/prepare_count and power-domain state so that these are
>>> not automatically disabled late in boot.
>>>
>>> If bootloader is enabling display, and kernel is using efifb before
>>> real display driver is loaded (potentially from kernel module after
>>> userspace starts, in a typical distro kernel), we don't want to kill
>>> the clocks and power domains that are used by the display before
>>> userspace starts.
>>>
>>> Second part is for drm/msm to check if display related clocks are
>>> enabled when it is loaded, and if so read back hw state to sync
>>> existing display state w/ software state, and skip the initial
>>> clk_enable's and otherwise fixing up clk/regulator/etc ref counts
>>> (skipping the normal display-enable codepaths), therefore inheriting
>>> the enable done by bootloader.
>>>
>>> Obviously this should be split up into multiple patches and many
>>> TODOs addressed.  But I guess this is enough for now to start
>>> discussing the approach, and in particular how drm and clock/pd
>>> drivers work together to handle handover from bootloader.
>>>
>>> The CLK_INHERIT_BOOTLOADER and related gsdc flag should only be set
>>> on leaf nodes.
>>> ---
>>
>> []..
>>
>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>> index d523991c945f..90b698c910d0 100644
>>> --- a/drivers/clk/qcom/common.c
>>> +++ b/drivers/clk/qcom/common.c
>>> @@ -11,6 +11,7 @@
>>>    * GNU General Public License for more details.
>>>    */
>>>
>>> +#include <linux/clk.h>
>>>   #include <linux/export.h>
>>>   #include <linux/module.h>
>>>   #include <linux/regmap.h>
>>> @@ -258,6 +259,33 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>>>        if (ret)
>>>                return ret;
>>>
>>> +     /* Check which of clocks that we inherit state from bootloader
>>> +      * are enabled, and fixup enable/prepare state (as well as that
>>> +      * of it's parents).
>>> +      *
>>> +      * TODO can we assume that parents coming from another clk
>>> +      * driver are already registered?
>>> +      */
>>> +     for (i = 0; i < num_clks; i++) {
>>> +             struct clk_hw *hw;
>>> +
>>> +             if (!rclks[i])
>>> +                     continue;
>>> +
>>> +             hw = &rclks[i]->hw;
>>> +
>>> +             if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
>>> +                     continue;
>>> +
>>> +             if (!clk_is_enabled_regmap(hw))
>>> +                     continue;
>>> +
>>> +             dev_dbg(dev, "%s is enabled from bootloader!\n",
>>> +                       hw->init->name);
>>> +
>>> +             clk_inherit_enabled(hw->clk);
>>
>> how about just calling a clk_prepare_enable(hw->clk) instead of adding a new API?
>> The flag could also be something qcom specific and then we avoid having to add
>> anything in generic CCF code and its all handled in the qcom clock drivers.
> 
> Hmm, I could try that..  I *guess* it doesn't hurt to enable an
> already enabled clk..

yes, ideally it shouldn't hurt.

> 
> beyond that, I wonder if this is something that other platforms would
> want, so maybe I should be doing the check for enabled in CCF core?
> If not, making this a qcom specific flag makes sense.

I think most previous attempts to solve this were done trying to keep
it very generic and they didn't go too far.
So I was thinking maybe we could deal with it within qcom drivers for
now, and if others come up with something similar, then look to
consolidate at a generic CCF level.

> 
>>> +     }
>>> +
>>>        reset = &cc->reset;
>>>        reset->rcdev.of_node = dev->of_node;
>>>        reset->rcdev.ops = &qcom_reset_ops;
>>
>> [] ..
>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index a4f3580587b7..440d819b2d9d 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -291,6 +291,12 @@ static int gdsc_init(struct gdsc *sc)
>>>        if ((sc->flags & VOTABLE) && on)
>>>                gdsc_enable(&sc->pd);
>>>
>>> +     if ((sc->flags & INHERIT_BL) && on) {
>>> +             pr_debug("gdsc: %s is enabled from bootloader!\n", sc->pd.name);
>>> +             gdsc_enable(&sc->pd);
>>> +             sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
>>
>> Would this not prevent the powerdomain from ever getting disabled?
> 
> Yeah, this is a hack, because I couldn't figure out something better.
> The problem is that gdsc/powerdomain doesn't have it's own
> enable_count but this is tracked at the device level (afaict.. maybe
> I'm miss-understanding something).  I guess we could add our own
> enable_count within gdsc?  Suggestions welcome, since I don't know
> these parts of the code so well.

One way to deal with it would be to tell the genpd core, that the gdsc
isn't really enabled, so it does not go ahead and disable it thinking
its unused.

Once the display driver comes up, it runtime enables/disables it.
I am guessing, if the bootloader turns on the gdsc, there will also
be a kernel driver to handle it.

So I am basically saying,
         if ((sc->flags & INHERIT_BL) && on)
                 on = !on; /* Prevent genpd from thinking its unsued */

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd July 18, 2017, 11:16 p.m. UTC | #4
On 07/17, Nayak, Rajendra wrote:
> 
> 
> On 7/14/2017 4:13 PM, Rob Clark wrote:
> >On Fri, Jul 14, 2017 at 12:52 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> >>Hi Rob,
> >>
> >>On 07/11/2017 11:50 PM, Rob Clark wrote:
> >>
> >>>diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >>>index d523991c945f..90b698c910d0 100644
> >>>--- a/drivers/clk/qcom/common.c
> >>>+++ b/drivers/clk/qcom/common.c
> >>>@@ -11,6 +11,7 @@
> >>>   * GNU General Public License for more details.
> >>>   */
> >>>
> >>>+#include <linux/clk.h>
> >>>  #include <linux/export.h>
> >>>  #include <linux/module.h>
> >>>  #include <linux/regmap.h>
> >>>@@ -258,6 +259,33 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>>+     /* Check which of clocks that we inherit state from bootloader
> >>>+      * are enabled, and fixup enable/prepare state (as well as that
> >>>+      * of it's parents).
> >>>+      *
> >>>+      * TODO can we assume that parents coming from another clk
> >>>+      * driver are already registered?
> >>>+      */
> >>>+     for (i = 0; i < num_clks; i++) {
> >>>+             struct clk_hw *hw;
> >>>+
> >>>+             if (!rclks[i])
> >>>+                     continue;
> >>>+
> >>>+             hw = &rclks[i]->hw;
> >>>+
> >>>+             if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
> >>>+                     continue;
> >>>+
> >>>+             if (!clk_is_enabled_regmap(hw))
> >>>+                     continue;
> >>>+
> >>>+             dev_dbg(dev, "%s is enabled from bootloader!\n",
> >>>+                       hw->init->name);
> >>>+
> >>>+             clk_inherit_enabled(hw->clk);
> >>
> >>how about just calling a clk_prepare_enable(hw->clk) instead of adding a new API?
> >>The flag could also be something qcom specific and then we avoid having to add
> >>anything in generic CCF code and its all handled in the qcom clock drivers.
> >
> >Hmm, I could try that..  I *guess* it doesn't hurt to enable an
> >already enabled clk..
> 
> yes, ideally it shouldn't hurt.

It hurts when you enable a PLL when it's already enabled. I
recall having to add code specifically for this reason to avoid
enabling a PLL that is already enabled out of the bootloader. But
that's because the enable sequence for a PLL is multiple writes
that first keep the output off and then turn it back on after
things stabilize. For RCGs this isn't the case.

> 
> >
> >beyond that, I wonder if this is something that other platforms would
> >want, so maybe I should be doing the check for enabled in CCF core?
> >If not, making this a qcom specific flag makes sense.
> 
> I think most previous attempts to solve this were done trying to keep
> it very generic and they didn't go too far.
> So I was thinking maybe we could deal with it within qcom drivers for
> now, and if others come up with something similar, then look to
> consolidate at a generic CCF level.

We know that other SoC vendors need handoff features as well, so
there will definitely be a point where we need to consolidate at
the framework level. Working something up that works for qcom
would be good to try out though to find edge cases, etc.
diff mbox

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fc58c52a26b4..7c84e8d6e816 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -796,6 +796,24 @@  static void clk_disable_unused_subtree(struct clk_core *core)
 		clk_core_disable_unprepare(core->parent);
 }
 
+/* clock and it's parents are already prepared/enabled from bootloader,
+ * so simply record the fact.
+ */
+static void __clk_inherit_enabled(struct clk_core *core)
+{
+	core->enable_count++;
+	core->prepare_count++;
+if (core->hw->init->name) // XXX hack.. something funny with xo/xo_board..
+	if (core->parent)
+		__clk_inherit_enabled(core->parent);
+}
+
+void clk_inherit_enabled(struct clk *clk)
+{
+	__clk_inherit_enabled(clk->core);
+}
+EXPORT_SYMBOL_GPL(clk_inherit_enabled);
+
 static bool clk_ignore_unused;
 static int __init clk_ignore_unused_setup(char *__unused)
 {
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index d523991c945f..90b698c910d0 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -11,6 +11,7 @@ 
  * GNU General Public License for more details.
  */
 
+#include <linux/clk.h>
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
@@ -258,6 +259,33 @@  int qcom_cc_really_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
+	/* Check which of clocks that we inherit state from bootloader
+	 * are enabled, and fixup enable/prepare state (as well as that
+	 * of it's parents).
+	 *
+	 * TODO can we assume that parents coming from another clk
+	 * driver are already registered?
+	 */
+	for (i = 0; i < num_clks; i++) {
+		struct clk_hw *hw;
+
+		if (!rclks[i])
+			continue;
+
+		hw = &rclks[i]->hw;
+
+		if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
+			continue;
+
+		if (!clk_is_enabled_regmap(hw))
+			continue;
+
+		dev_dbg(dev, "%s is enabled from bootloader!\n",
+			  hw->init->name);
+
+		clk_inherit_enabled(hw->clk);
+	}
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;
diff --git a/drivers/clk/qcom/gcc-msm8916.c b/drivers/clk/qcom/gcc-msm8916.c
index 2cfe7000fc60..c939d33f6377 100644
--- a/drivers/clk/qcom/gcc-msm8916.c
+++ b/drivers/clk/qcom/gcc-msm8916.c
@@ -2468,7 +2468,7 @@  static struct clk_branch gcc_mdss_ahb_clk = {
 				"pcnoc_bfdcd_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2485,7 +2485,7 @@  static struct clk_branch gcc_mdss_axi_clk = {
 				"system_noc_bfdcd_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2502,7 +2502,7 @@  static struct clk_branch gcc_mdss_byte0_clk = {
 				"byte0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2519,7 +2519,7 @@  static struct clk_branch gcc_mdss_esc0_clk = {
 				"esc0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2536,7 +2536,7 @@  static struct clk_branch gcc_mdss_mdp_clk = {
 				"mdp_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2553,7 +2553,7 @@  static struct clk_branch gcc_mdss_pclk0_clk = {
 				"pclk0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -2570,7 +2570,7 @@  static struct clk_branch gcc_mdss_vsync_clk = {
 				"vsync_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -3060,6 +3060,7 @@  static struct gdsc mdss_gdsc = {
 		.name = "mdss",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = INHERIT_BL,
 };
 
 static struct gdsc jpeg_gdsc = {
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580587b7..440d819b2d9d 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -291,6 +291,12 @@  static int gdsc_init(struct gdsc *sc)
 	if ((sc->flags & VOTABLE) && on)
 		gdsc_enable(&sc->pd);
 
+	if ((sc->flags & INHERIT_BL) && on) {
+		pr_debug("gdsc: %s is enabled from bootloader!\n", sc->pd.name);
+		gdsc_enable(&sc->pd);
+		sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
+	}
+
 	if (on || (sc->pwrsts & PWRSTS_RET))
 		gdsc_force_mem_on(sc);
 	else
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 39648348e5ec..3b5e64b060c2 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -53,6 +53,7 @@  struct gdsc {
 #define VOTABLE		BIT(0)
 #define CLAMP_IO	BIT(1)
 #define HW_CTRL		BIT(2)
+#define INHERIT_BL	BIT(3)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c59c62571e4f..4d5505f92329 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -35,6 +35,7 @@ 
 #define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
 /* parents need enable during gate/ungate, set rate and re-parent */
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
+#define CLK_INHERIT_BOOTLOADER	BIT(13) /* clk may be enabled from bootloader */
 
 struct clk;
 struct clk_hw;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 91bd464f4c9b..461991fc57e2 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -391,6 +391,15 @@  void clk_disable(struct clk *clk);
 void clk_bulk_disable(int num_clks, const struct clk_bulk_data *clks);
 
 /**
+ * clk_inherit_enabled - update the enable/prepare count of a clock and it's
+ * parents for clock enabled by bootloader.
+ *
+ * Intended to be used by clock drivers to inform the clk core of a clock
+ * that is already running.
+ */
+void clk_inherit_enabled(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source