Re: [PATCH] soc: mediatek: Fix random hang up issue while kernel init

Re: [PATCH] soc: mediatek: Fix random hang up issue while kernel init

[Daniel Kurtz]

+linux-clk +linux-pm and the genpd and clock gurus

Hi James,

On Thu, Oct 1, 2015 at 5:26 PM, James Liao <jamesjj.liao@mediatek.com> wrote:
> Hi Daniel,
>
> On Thu, 2015-10-01 at 16:07 +0800, Daniel Kurtz wrote:
>> Hmm... below is my current understanding.
>>
>> In the current software architecture, we have split the "venc"
>> subsystem register space into two parts (two dt nodes), with each node
>> handled instantiated as its own  platform device, and handled by a
>> different platform driver:
>>
>> vencsys: clock-controller@18000000 {
>>   compatible = "mediatek,mt8173-vencsys", "syscon";
>>   reg = <0 0x18000000 0 0x1000>;
>>   #clock-cells = <1>;
>> };
>>
>> /* TBD - a venc driver has not been posted to the list so I don't
>> really know yet what it will look like */
>>  venc: encoder@~18000000 {
>>    compatible = "mediatek,mt8173-venc?";
>>    reg = <0 ~0x18000000 0 ?>;
>>    clocks = <& <&vencsys CLK_VENC_CKE1>, <&vencsys CLK_VENC_CKE0>, ...;
>>    clock-names = "apb", "smi", ...;
>>    ...
>>  };
>>
>> Each independent driver must take appropriate independent action to
>> ensure that any clock required to access its associated registers is
>> enabled when accessing said registers.
>> In other words,
>>  * the venc *clock-controller* driver that must enable any clocks
>> required to access the *clock control* registers
>>  * the venc *encoder* driver must enable clocks any clocks required to
>> access the *encoder* registers
>
>> Actually, the situation is a little worse that this, since we also
>> have a 'larb' node in the same register space, whose driver must also
>> ensure that VENC_SEL is enabled before accessing its registers:
>>
>> larb3: larb@18001000 {
>>   compatible = "mediatek,mt8173-smi-larb";
>>   reg = <0 0x18001000 0 0x1000>;
>>   mediatek,smi = <&smi_common>;
>>   power-domains = <&scpsys MT8173_POWER_DOMAIN_VENC>;
>>   clocks = <&vencsys CLK_VENC_CKE1>, <&vencsys CLK_VENC_CKE0>;
>>   clock-names = "apb", "smi";
>> };
>>
>> Personally, I am not opposed to using the power-domain enable/disable
>> as a proxy for also enabling/disabling the "subsystem register bank"
>> clock as is done in this patch and by the scpsys driver in general.
>> However, it feels a bit like an abuse of the power domain api.
>>
>> > I preferred to keep venc_sel on during VENC power on because I'm not
>> > sure there is any other framework may control VENC's registers during
>> > kernel init.
>>
>> The misunderstanding here is the interpretation of "VENC's registers".
>> In this case, the registers in question are those owned by the
>> "vencsys: clock-controller@18000000" node, and the driver accessing
>> them is drivers/clk/mediatek/clk-gate.c.
>
> The situation is more complex than you mentioned above. VENC (0x18000000
> ~ 0x1800ffff) for example, if we want to avoid hanging up while
> accessing registers, we need to:
>
>  - Prevent venc_sel off while VENC is powered on.
>  - Prevent mm_sel off while MM is powered on.
>
> First, it's not worth to control power domains in clock control path
> because it's too expensive. So we want to keep clock on before accessing
> registers or during the domain is powered on.
>
> Besides, modeling a clock controller as a consumer of power domain may
> not a good idea, because there are power domains be consumers of clocks,
> such as:
>
>         [MT8173_POWER_DOMAIN_MM] = {
>                 .name = "mm",
>                 [...]
>                 .clk_id = MT8173_CLK_MM,
>                 [...]
>         },

I see two cases where "a power domain is a consumer of a clock":
  (a) the clock is needed to access the power domain control
registers.  The clock must actually be in another power domain, since
otherwise you could never turn *on* the power domain in question.
  (b) the clock is needed to talk to the power domain that is about to
turn off, or that was just turned on - these clocks are usually used
to control some kind of bus arbitration, etc.  In this case, the
clocks are only accessed when the power domain is on.

I would argue that in both cases, the clock should in theory should
only be enabled/disabled by the power on/off routine for the duration
of time that is needed, and that it should not be "left enabled" by
the power domain on/off function...  I can see how this might be a
useful optimization - but it also seems like a way to burn extra power
by leaving on a clock that is not necessarily being used.

But - perhaps I am over thinking this, and it is standard practice to
bundle power domains with the clocks that service components in the
power domain.

> Second, if we want to avoid hanging up by enabling related top clocks in
> clock controllers, we need to clock on venc_sel and mm_sel before *each
> clock operations*, and then clock off them after clock operations. That
> means:
>
> - To check venc_cke0's state:
>  = clk_prepare mm_sel and venc_sel (where to prepare?)
>   - It may turn on related PLLs, and it can't be invoked in an atomic
> context.
>  = clk_enable mm_sel and venc_sel.
>  = Read VENC's clock register.
>  = clk_disable mm_sel and venc_sel.
>  = clk_unprepare mm_sel and venc_sel (where to unprepare?)
>   - It may turn off PLLs, and it can't be invoked in an atomic context.
>  = Return venc_cke0's state.
>
> Overhead is a reason. The clock framework's API flow (prepare - enable -
> disable - unprepare) is another reason to reject the solution in clock
> controllers, because prepare/unprepare is a non-atomic operation but
> operations to access registers may be a atomic context.

Or, alternatively, just prepare venc_sel once during init, for each
clock controller that needs it, when configuring the clock controller
node (for example, in mtk_vencsys_init()). This is similar to how we
set up 'critical clocks'.
By just preparing the clock, you tell the common clock core:
 "my clock controller will need this clock later; please make sure
that it can be enabled/disabled later during atomic context."

Thanks!
-Dan

>
>
>
> Best regards,
>
> James
>
>
>

Re: [PATCH] soc: mediatek: Fix random hang up issue while kernel init

[James Liao]

Hi Daniel,

On Thu, 2015-10-01 at 18:08 +0800, Daniel Kurtz wrote:
> I see two cases where "a power domain is a consumer of a clock":
>   (a) the clock is needed to access the power domain control
> registers.  The clock must actually be in another power domain, since
> otherwise you could never turn *on* the power domain in question.
>   (b) the clock is needed to talk to the power domain that is about to
> turn off, or that was just turned on - these clocks are usually used
> to control some kind of bus arbitration, etc.  In this case, the
> clocks are only accessed when the power domain is on.
> 
> I would argue that in both cases, the clock should in theory should
> only be enabled/disabled by the power on/off routine for the duration
> of time that is needed, and that it should not be "left enabled" by
> the power domain on/off function...  I can see how this might be a
> useful optimization - but it also seems like a way to burn extra power
> by leaving on a clock that is not necessarily being used.
> 
> But - perhaps I am over thinking this, and it is standard practice to
> bundle power domains with the clocks that service components in the
> power domain.

Yes, you are right. Power domain on/off operations need clocks to get
status of the subsystem. Keeping clocks on during power domain on is an
optimization.

But to model a clock controller as a power domain consumer, or say we
need to control power domain before clock on/off, is not a good practice
because we always consider clock operations should be light weight.
Power domains should be controlled explicitly by subsystem drivers,
instead of hiding behind clock controls.

> Or, alternatively, just prepare venc_sel once during init, for each
> clock controller that needs it, when configuring the clock controller
> node (for example, in mtk_vencsys_init()). This is similar to how we
> set up 'critical clocks'.
> By just preparing the clock, you tell the common clock core:
>  "my clock controller will need this clock later; please make sure
> that it can be enabled/disabled later during atomic context."

In current implementation, PLLs will be turned on in clk_prepare(). So
if a clock is always prepared, its parent PLL will be kept on even if
the clock is not enabled. It consumes extra power, so we don't prefer
this kind of solution.


Best regards,

James

Re: [PATCH] soc: mediatek: Fix random hang up issue while kernel init

[Daniel Kurtz]

On Fri, Oct 2, 2015 at 11:00 AM, James Liao <jamesjj.liao@mediatek.com> wrote:
> Hi Daniel,
>
> On Thu, 2015-10-01 at 18:08 +0800, Daniel Kurtz wrote:
>> I see two cases where "a power domain is a consumer of a clock":
>>   (a) the clock is needed to access the power domain control
>> registers.  The clock must actually be in another power domain, since
>> otherwise you could never turn *on* the power domain in question.
>>   (b) the clock is needed to talk to the power domain that is about to
>> turn off, or that was just turned on - these clocks are usually used
>> to control some kind of bus arbitration, etc.  In this case, the
>> clocks are only accessed when the power domain is on.
>>
>> I would argue that in both cases, the clock should in theory should
>> only be enabled/disabled by the power on/off routine for the duration
>> of time that is needed, and that it should not be "left enabled" by
>> the power domain on/off function...  I can see how this might be a
>> useful optimization - but it also seems like a way to burn extra power
>> by leaving on a clock that is not necessarily being used.
>>
>> But - perhaps I am over thinking this, and it is standard practice to
>> bundle power domains with the clocks that service components in the
>> power domain.
>
> Yes, you are right. Power domain on/off operations need clocks to get
> status of the subsystem. Keeping clocks on during power domain on is an
> optimization.
>
> But to model a clock controller as a power domain consumer, or say we
> need to control power domain before clock on/off, is not a good practice
> because we always consider clock operations should be light weight.
> Power domains should be controlled explicitly by subsystem drivers,
> instead of hiding behind clock controls.
>
>> Or, alternatively, just prepare venc_sel once during init, for each
>> clock controller that needs it, when configuring the clock controller
>> node (for example, in mtk_vencsys_init()). This is similar to how we
>> set up 'critical clocks'.
>> By just preparing the clock, you tell the common clock core:
>>  "my clock controller will need this clock later; please make sure
>> that it can be enabled/disabled later during atomic context."
>
> In current implementation, PLLs will be turned on in clk_prepare(). So
> if a clock is always prepared, its parent PLL will be kept on even if
> the clock is not enabled. It consumes extra power, so we don't prefer
> this kind of solution.

Actually, I should have proposed adding prepare / unprepare callbacks
to mtk_clk_gate_ops in which we prepare_enable/disable_unprepare
venc_sel (& mm_sel).
This should correctly track all of the clk reference counting needed
to access the venc clock control registers.


However, this would not actually solve the issue that this patch is
trying to fix, since nobody would have called the
prepare_clk(venc_cke0) before clk_disable_unused_subtree(venc_cke0).
Bummer.

I think the crux of the original issue is that the mediatek/clk-gate.c
is_enabled() callbacks may be called at a point in time when venc_sel
is disabled but VENC PD is enabled.
So, perhaps an easier, but still a little hacky way out is to keep the
clk & PD problems separate, and just check __clk_is_enabled(venc_sel)
before reading the venc clk status register.

WDYT?

By the way, I'm not really opposed to your current implementation
either, I was just helping Matthias & Lucas understand in enough
detail that they can help review.

-Dan

>
>
> Best regards,
>
> James
>
>

Re: [PATCH] soc: mediatek: Fix random hang up issue while kernel init

[James Liao]

HI Daniel,

On Fri, 2015-10-02 at 17:25 +0800, Daniel Kurtz wrote:
> Actually, I should have proposed adding prepare / unprepare callbacks
> to mtk_clk_gate_ops in which we prepare_enable/disable_unprepare
> venc_sel (& mm_sel).
> This should correctly track all of the clk reference counting needed
> to access the venc clock control registers.
> 
> 
> However, this would not actually solve the issue that this patch is
> trying to fix, since nobody would have called the
> prepare_clk(venc_cke0) before clk_disable_unused_subtree(venc_cke0).
> Bummer.

Hmm... it's true.

> I think the crux of the original issue is that the mediatek/clk-gate.c
> is_enabled() callbacks may be called at a point in time when venc_sel
> is disabled but VENC PD is enabled.
> So, perhaps an easier, but still a little hacky way out is to keep the
> clk & PD problems separate, and just check __clk_is_enabled(venc_sel)
> before reading the venc clk status register.
> 
> WDYT?

This may be a way to resolve this issue, but model the prerequisite
clock of subsystem clocks is not straightforward. And once we model the
prerequisite clocks, someone may suggest us to enable prerequisite
clocks before operate subsystem clocks, which may have concerns on
overhead and complexity.

> By the way, I'm not really opposed to your current implementation
> either, I was just helping Matthias & Lucas understand in enough
> detail that they can help review.

I know and many thanks for your suggestion on this patch.


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html