| Message ID | 20230531-topic-8998_mmssclk-v3-0-ba1b1fd9ee75@linaro.org |
|---|---|
| Headers | show |
| Series | MMCC MSM8998 fixes | expand |
On 8/9/2023 1:20 PM, Konrad Dybcio wrote: > The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the > HW_CTRL flag (it's separate from hw_ctrl_addr). In addition to that, > it should feature a cxc entry for bimc_smmu_axi_clk and be marked as > votable. I appear to have confused HW_CTRL with hw_ctrl_addr. Thanks for fixing that. I recall I made it always-on for display handoff. The bootloader on the laptops will enable the display, which means the MDP is active and using the SMMU. The SMMU is powered by the GDSC as you know. The MDP is going to be polling a framebuffer in DDR, which EFI services (efifb) is going to be updating. All of this is active during linux boot, which is how the kernel bootlog gets printed on screen. If I remember right, the GDSC will be registered. When it is done probing, there will be no consumers. So the Linux framework will step in and turn it off before the consumers come up. This kills power to the SMMU. If the SMMU doesn't come back on before the MDP polls DDR again, you get a bus hang and a crash. I assumed that any msm8998 device would be using the MDP/GPU and thus the SMMU would pretty much always be powered on. I expected this patch to break the laptop. It does not in my testing. However, I see that I disabled the MMCC node in DT with a todo about the display. So the GDSC is never registered, and then never gets turned off. I believe that todo is pending some updates I need to make to the TI DSI/eDP bridge because the I2C port on the bridge is not wired up. I should really dust that off and complete it. Regardless, even with the todo addressed, I think removing always-on will still break the laptops unless the bootloader handoff of display was solved and I missed it. I get that for your usecase, a phone where the bootloader does not init the display, always-on has the potential to burn extra power. I'm not sure how to make both of us happy through. Do you have any suggestions? > > Fix all of these issues. > > Fixes: d14b15b5931c ("clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/mmcc-msm8998.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c > index d0a5440e2291..4fdc41e7d2a8 100644 > --- a/drivers/clk/qcom/mmcc-msm8998.c > +++ b/drivers/clk/qcom/mmcc-msm8998.c > @@ -2627,11 +2627,13 @@ static struct gdsc camss_cpp_gdsc = { > static struct gdsc bimc_smmu_gdsc = { > .gdscr = 0xe020, > .gds_hw_ctrl = 0xe024, > + .cxcs = (unsigned int []){ 0xe008 }, > + .cxc_count = 1, > .pd = { > .name = "bimc_smmu", > }, > .pwrsts = PWRSTS_OFF_ON, > - .flags = HW_CTRL | ALWAYS_ON, > + .flags = VOTABLE, > }; > > static struct clk_regmap *mmcc_msm8998_clocks[] = { >
On 10.08.2023 20:20, Jeffrey Hugo wrote: > On 8/9/2023 1:20 PM, Konrad Dybcio wrote: >> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the >> HW_CTRL flag (it's separate from hw_ctrl_addr). In addition to that, >> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as >> votable. > > I appear to have confused HW_CTRL with hw_ctrl_addr. Thanks for fixing that. > > I recall I made it always-on for display handoff. The bootloader on the laptops will enable the display, which means the MDP is active and using the SMMU. The SMMU is powered by the GDSC as you know. The MDP is going to be polling a framebuffer in DDR, which EFI services (efifb) is going to be updating. All of this is active during linux boot, which is how the kernel bootlog gets printed on screen. This is essentially a missing / mis-configuration from the linux/dt POV and I think the consensus for using display without describing it properly with mdss has been to do one of: - adding a simple-framebuffer node with all the necessary clocks/pds - adding "clk_ignore_unused pd_ignore_unused" to your cmdline > > If I remember right, the GDSC will be registered. When it is done probing, there will be no consumers. So the Linux framework will step in and turn it off before the consumers come up. This kills power to the SMMU. If the SMMU doesn't come back on before the MDP polls DDR again, you get a bus hang and a crash. Yep > I assumed that any msm8998 device would be using the MDP/GPU and thus the SMMU would pretty much always be powered on. This flag however bans putting it to sleep when not in use. > > I expected this patch to break the laptop. It does not in my testing. However, I see that I disabled the MMCC node in DT with a todo about the display. So the GDSC is never registered, and then never gets turned off. I believe that todo is pending some updates I need to make to the TI DSI/eDP bridge because the I2C port on the bridge is not wired up. I should really dust that off and complete it. Right, so what you have now is a third, untold "solution" to the problem described above.. not really a supported configuration as it's not "correct" I'd happily see you wire up the bridge et al though! > Regardless, even with the todo addressed, I think removing always-on will still break the laptops unless the bootloader handoff of display was solved and I missed it. > > I get that for your usecase, a phone where the bootloader does not init the display, always-on has the potential to burn extra power. I'm not sure how to make both of us happy through. > > Do you have any suggestions? Hope my replies above are enough. Konrad
On 8/10/2023 12:46 PM, Konrad Dybcio wrote: > On 10.08.2023 20:20, Jeffrey Hugo wrote: >> On 8/9/2023 1:20 PM, Konrad Dybcio wrote: >>> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the >>> HW_CTRL flag (it's separate from hw_ctrl_addr). In addition to that, >>> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as >>> votable. >> >> I appear to have confused HW_CTRL with hw_ctrl_addr. Thanks for fixing that. >> >> I recall I made it always-on for display handoff. The bootloader on the laptops will enable the display, which means the MDP is active and using the SMMU. The SMMU is powered by the GDSC as you know. The MDP is going to be polling a framebuffer in DDR, which EFI services (efifb) is going to be updating. All of this is active during linux boot, which is how the kernel bootlog gets printed on screen. > This is essentially a missing / mis-configuration from the linux/dt POV and > I think the consensus for using display without describing it properly with > mdss has been to do one of: > > - adding a simple-framebuffer node with all the necessary clocks/pds > - adding "clk_ignore_unused pd_ignore_unused" to your cmdline > >> >> If I remember right, the GDSC will be registered. When it is done probing, there will be no consumers. So the Linux framework will step in and turn it off before the consumers come up. This kills power to the SMMU. If the SMMU doesn't come back on before the MDP polls DDR again, you get a bus hang and a crash. > Yep > >> I assumed that any msm8998 device would be using the MDP/GPU and thus the SMMU would pretty much always be powered on. > This flag however bans putting it to sleep when not in use. > >> >> I expected this patch to break the laptop. It does not in my testing. However, I see that I disabled the MMCC node in DT with a todo about the display. So the GDSC is never registered, and then never gets turned off. I believe that todo is pending some updates I need to make to the TI DSI/eDP bridge because the I2C port on the bridge is not wired up. I should really dust that off and complete it. > Right, so what you have now is a third, untold "solution" to the problem > described above.. not really a supported configuration as it's not "correct" > > I'd happily see you wire up the bridge et al though! > > >> Regardless, even with the todo addressed, I think removing always-on will still break the laptops unless the bootloader handoff of display was solved and I missed it. >> >> I get that for your usecase, a phone where the bootloader does not init the display, always-on has the potential to burn extra power. I'm not sure how to make both of us happy through. >> >> Do you have any suggestions? > Hope my replies above are enough. I still think there is an issue, but my setup is not as complete as your on mainline. I'll clean things up and we'll solve the issues when we get to them. Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote: > 8998 has a couple of issues related to its clock controllers. This series > attemps to fix some of them. > > The DT patch should go in first for bisectability, otherwise > clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer > considered always-on. > > [...] Applied bindings patch to will (for-joerg/arm-smmu/bindings), thanks! [6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description https://git.kernel.org/will/c/b606e2e8eded Cheers,
On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote: > 8998 has a couple of issues related to its clock controllers. This series > attemps to fix some of them. > > The DT patch should go in first for bisectability, otherwise > clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer > considered always-on. > > [...] Applied, thanks! [3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks commit: a6f1e8623836bb6ce64c347d110ecb6259ae73c4 [4/6] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks (no commit info) Best regards,
On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote: > 8998 has a couple of issues related to its clock controllers. This series > attemps to fix some of them. > > The DT patch should go in first for bisectability, otherwise > clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer > considered always-on. > > [...] Applied, thanks! [3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks (no commit info) [4/6] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks commit: 9906c4140897bbdbff7bb71c6ae67903cb9954ce [5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC commit: 1fc62c8347397faf4e18249e88ecd4470c0a5357 Best regards,
8998 has a couple of issues related to its clock controllers. This series attemps to fix some of them. The DT patch should go in first for bisectability, otherwise clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer considered always-on. This series results in less "clk stuck at 'on/off'" messages and should marginally reduce power consumption. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Changes in v3: - Make the commit message more meaningful in patch 6 (krzk) - Pick up tags - Link to v2: https://lore.kernel.org/r/20230531-topic-8998_mmssclk-v2-0-34273e275c51@linaro.org Changes in v2: - Update bindings - Separate out the dt patch into two - Pick up tags - Link to v1: https://lore.kernel.org/r/20230531-topic-8998_mmssclk-v1-0-2b5a8fc90991@linaro.org --- Konrad Dybcio (6): arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU arm64: dts: qcom: msm8998: Add missing power domain to MMSS SMMU clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks clk: qcom: mmcc-msm8998: Fix the SMMU GDSC dt-bindings: arm-smmu: Fix MSM8998 clocks description .../devicetree/bindings/iommu/arm,smmu.yaml | 41 ++++++++++++++++++++++ arch/arm64/boot/dts/qcom/msm8998.dtsi | 8 +++-- drivers/clk/qcom/gcc-msm8998.c | 6 ++-- drivers/clk/qcom/mmcc-msm8998.c | 7 +++- 4 files changed, 55 insertions(+), 7 deletions(-) --- base-commit: 21ef7b1e17d039053edaeaf41142423810572741 change-id: 20230531-topic-8998_mmssclk-fd4b2fb7f8c1 Best regards,