RE: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Sricharan]

Hi Stan,

>-----Original Message-----
>From: Stanimir Varbanov [mailto:stanimir.varbanov@xxxxxxxxxx]
>Sent: Wednesday, January 11, 2017 2:25 PM
>To: Sricharan <sricharan@xxxxxxxxxxxxxx>; 'Stanimir Varbanov' <stanimir.varbanov@xxxxxxxxxx>; 'Rajendra Nayak'
><rnayak@xxxxxxxxxxxxxx>; sboyd@xxxxxxxxxxxxxx; mturquette@xxxxxxxxxxxx
>Cc: linux-clk@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>
>Hi Sricharan,
>
>On 01/10/2017 09:29 PM, Sricharan wrote:
>> Hi stan,
>>
>>> -----Original Message-----
>>> From: linux-arm-msm-owner@xxxxxxxxxxxxxxx [mailto:linux-arm-msm-owner@xxxxxxxxxxxxxxx] On Behalf Of Stanimir Varbanov
>>> Sent: Tuesday, January 10, 2017 10:14 PM
>>> To: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>; sboyd@xxxxxxxxxxxxxx; mturquette@xxxxxxxxxxxx
>>> Cc: linux-clk@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; sricharan@xxxxxxxxxxxxxx
>>> Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>>>
>>> Hi Rajendra,
>>>
>>> On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>>>> Once a gdsc is brought in and out of HW control, there is a
>>>> power down and up cycle which can take upto 1us. Polling on
>>>> the gdsc status immediately after the hw control enable/disable
>>>> can mislead software/firmware to belive the gdsc is already either on
>>>> or off, while its yet to complete the power cycle.
>>>> To avoid this add a 1us delay post a enable/disable of HW control
>>>> mode.
>>>>
>>>> Also after the HW control mode is disabled, poll on the status to
>>>> check gdsc status reflects its 'on' before force disabling it
>>>> in software.
>>>>
>>>> Reported-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
>>>> Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
>>>> ---
>>>>
>>>> Stan,
>>>> If there was a specific issue you saw with venus because of the missing
>>>> delay and poll, can you check if this fixes any of that.
>>>
>>> Something more about the issue.
>>>
>>> I had re-designed venus driver on three platform drivers venus-core,
>>> venus-dec and venus-enc in order to be able to control those three
>>> power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>>>
>>> After that I abstracted MMAGIC hw on a separate mmagic driver. This
>>> driver just controls mmagic clocks and GDSC in its runtime_suspend and
>>> runtime_resume methods.
>>>
>>> The DT nodes looks like:
>>>
>>> mmagic_video {
>>> 	compatible = "qcom,msm8996-mmagic";
>>> 	clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_AHB_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
>>> 		 <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_MAXI_CLK>,
>>> 		 <&mmcc MMAGIC_VIDEO_AXI_CLK>,
>>> 		 <&mmcc SMMU_VIDEO_AHB_CLK>,
>>> 		 <&mmcc SMMU_VIDEO_AXI_CLK>;
>>> 	power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>>>
>>> 	video-codec {
>>> 		compatible = "qcom,msm8996-venus";
>>> 		clocks = <&mmcc VIDEO_CORE_CLK>,
>>> 			 <&mmcc VIDEO_AHB_CLK>,
>>> 			 <&mmcc VIDEO_AXI_CLK>,
>>> 			 <&mmcc VIDEO_MAXI_CLK>;
>>> 		power-domains = <&mmcc VENUS_GDSC>;
>>> 		...
>>>
>>> 		video-decoder {
>>> 			compatible = "venus-decoder";
>>> 			clocks = "subcore0";
>>> 			clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>>> 			power-domains = <&mmcc VENUS_CORE0_GDSC>;
>>> 		};
>>>
>>> 		video-encoder {
>>> 			compatible = "venus-encoder";
>>> 			clocks = "subcore1";
>>> 			clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>>> 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>>> 		};
>>> 	};
>>> };
>>>
>>> Note that mmagic_video is a parent dt node for smmu_video DT node so
>>> that clocks and mmagic_video gdsc will be enabled once smmu driver is
>>> instantiated by venus-core diriver.
>>>
>>
>> mmagic_video is a parent DT for smmu_video ? , so there are no clocks
>> populated for the smmu node as such ?
>
>Yes, I completely disabled runtime pm in the arm-smmu driver.
>

So completely disabling runtime pm in smmu has a downside because,
when the smmu is accessed standalone like, say in case of a fault,
would then fail. Anyways i think this is experimental probably.

>>
>>> Now when video-dec driver calls pm_runtime_get_sync() the sequence of
>>> enabling is:
>>>
>>> MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
>>> VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock
>>>
>>> When video-dec platform driver calls pm_runtime_put_sync() we should
>>> disabling of GDSC and clocks in the reversed oder.
>>>
>>> The issue comes when I have ran video decoder, the decoder hw finish
>>> stream decoding and we want to suspend venus core. The issue is that
>>> when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
>>> the system reboots.
>>>
>>> I have added a delay (200ms) before disabling mmagic clocks and then
>>> everything is fine again.
>>>
>>> Any idea?
>>>
>>
>> Can you share me a branch, i can have a quick check with a t32
>> if there is any crash logged in the TZ buffer when the system reboots.
>
>I can share a branch but you will need my initramfs too.
>
>I don't think it is tz related, most probably it is MMAGIC sequence
>issue or something in GDSC/MMCC.
>

I was not suspecting a TZ issue, but some crash because of which the board reboots and
debug msgs getting logged in the TZ log buffer.  Not sure if you already proceeded
further on this.

Regards,
 Sricharan