Hi Stan, >-----Original Message----- >From: Stanimir Varbanov [mailto:stanimir.varba...@linaro.org] >Sent: Wednesday, January 11, 2017 2:25 PM >To: Sricharan <sricha...@codeaurora.org>; 'Stanimir Varbanov' ><stanimir.varba...@linaro.org>; 'Rajendra Nayak' ><rna...@codeaurora.org>; sb...@codeaurora.org; mturque...@baylibre.com >Cc: linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org; >linux-kernel@vger.kernel.org >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-ow...@vger.kernel.org >>> [mailto:linux-arm-msm-ow...@vger.kernel.org] On Behalf Of Stanimir Varbanov >>> Sent: Tuesday, January 10, 2017 10:14 PM >>> To: Rajendra Nayak <rna...@codeaurora.org>; sb...@codeaurora.org; >>> mturque...@baylibre.com >>> Cc: linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org; >>> linux-kernel@vger.kernel.org; sricha...@codeaurora.org >>> 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.varba...@linaro.org> >>>> Signed-off-by: Rajendra Nayak <rna...@codeaurora.org> >>>> --- >>>> >>>> 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
