| Message ID | 1477304297-5248-4-git-send-email-sricharan@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 10/24, Sricharan R wrote: > With the venus subcore0/1 gdscs(powerdomains) in > hw controlled mode, the clock controller does not handle > the status bit for the clocks in that domain. So avoid > checking for the status bit of those clocks by setting the > BRANCH_HALT_DELAY flag. This avoids the WARN_ONs which > otherwise occurs when enabling/disabling those clocks. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> A better design would be to check if the associated GDSC is in hw control mode and then skip the checks because the clocks are no longer under the control of the registers. I presume we only enable the clocks here to turn on parent clocks which don't turn on/off automatically, i.e. the PLL. Given that hw control is a static decision I guess that is an over-engineered solution though? The problem is that this seems brittle because we have to keep two things in sync, the branches and the gdsc. So I guess this is ok, but it deserves a comment like "GDSC is in HW control" so we know what's going on. Also the commit text could be more explicit that clocks within the gdsc power domain don't work when the gdsc is off, and with hw control of a gdsc we can't tell when the gdsc may be off or on.
Hi, > >A better design would be to check if the associated GDSC is in hw >control mode and then skip the checks because the clocks are no >longer under the control of the registers. I presume we only >enable the clocks here to turn on parent clocks which don't turn >on/off automatically, i.e. the PLL. > I was thinking clocks in the powerdomain still needs to be turned on explicitly, these are branch clocks, irrespective of the PLLs. Putting the gdsc in hw_ctrl, only makes the polling on their status invalid. Anyways would be good to be aligned on this. >Given that hw control is a static decision I guess that is an >over-engineered solution though? The problem is that this seems >brittle because we have to keep two things in sync, the branches >and the gdsc. So I guess this is ok, but it deserves a comment >like "GDSC is in HW control" so we know what's going on. Also the >commit text could be more explicit that clocks within the gdsc >power domain don't work when the gdsc is off, and with hw control >of a gdsc we can't tell when the gdsc may be off or on. > ok, i will reword the commit log better as above. So i understand its ok to continue with this way of checking ? since we are always having a static association which never changes, than introducing additional fields in the clk_branch which can get the status of the gdsc. Regards, Sricharan -- 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
On 11/04, Sricharan wrote: > Hi, > > > >A better design would be to check if the associated GDSC is in hw > >control mode and then skip the checks because the clocks are no > >longer under the control of the registers. I presume we only > >enable the clocks here to turn on parent clocks which don't turn > >on/off automatically, i.e. the PLL. > > > I was thinking clocks in the powerdomain still needs to be turned > on explicitly, these are branch clocks, irrespective of the PLLs. > Putting the gdsc in hw_ctrl, only makes the polling on their status > invalid. Anyways would be good to be aligned on this. Sure. We also need to make sure the branches are on themselves. When the gdsc is disabled the clocks are killed though. This is why we can't enable clocks until the gdsc is enabled. > > >Given that hw control is a static decision I guess that is an > >over-engineered solution though? The problem is that this seems > >brittle because we have to keep two things in sync, the branches > >and the gdsc. So I guess this is ok, but it deserves a comment > >like "GDSC is in HW control" so we know what's going on. Also the > >commit text could be more explicit that clocks within the gdsc > >power domain don't work when the gdsc is off, and with hw control > >of a gdsc we can't tell when the gdsc may be off or on. > > > > ok, i will reword the commit log better as above. > > So i understand its ok to continue with this way of checking ? > since we are always having a static association which never changes, > than introducing additional fields in the clk_branch which can > get the status of the gdsc. > Well I'm also curious which case is failing. Does turning on the clocks work after the gdsc is enabled? Does turning off the clocks fail because we don't know when the gdsc has turned off? I would hope that the firmware keeps the gdsc on when it's done processing things, goes idle, and hands back control to software. Right now I'm failing to see how the halt bits fail to toggle assuming that firmware isn't misbehaving and the kernel driver is power controlling in a coordinated manner with the firmware.
On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote: > On 11/04, Sricharan wrote: >> Hi, >>> >>> A better design would be to check if the associated GDSC is in hw >>> control mode and then skip the checks because the clocks are no >>> longer under the control of the registers. I presume we only >>> enable the clocks here to turn on parent clocks which don't turn >>> on/off automatically, i.e. the PLL. >>> >> I was thinking clocks in the powerdomain still needs to be turned >> on explicitly, these are branch clocks, irrespective of the PLLs. >> Putting the gdsc in hw_ctrl, only makes the polling on their status >> invalid. Anyways would be good to be aligned on this. > > Sure. We also need to make sure the branches are on themselves. > When the gdsc is disabled the clocks are killed though. This is > why we can't enable clocks until the gdsc is enabled. > >> >>> Given that hw control is a static decision I guess that is an >>> over-engineered solution though? The problem is that this seems >>> brittle because we have to keep two things in sync, the branches >>> and the gdsc. So I guess this is ok, but it deserves a comment >>> like "GDSC is in HW control" so we know what's going on. Also the >>> commit text could be more explicit that clocks within the gdsc >>> power domain don't work when the gdsc is off, and with hw control >>> of a gdsc we can't tell when the gdsc may be off or on. >>> >> >> ok, i will reword the commit log better as above. >> >> So i understand its ok to continue with this way of checking ? >> since we are always having a static association which never changes, >> than introducing additional fields in the clk_branch which can >> get the status of the gdsc. >> > > Well I'm also curious which case is failing. Does turning on the > clocks work after the gdsc is enabled? Does turning off the > clocks fail because we don't know when the gdsc has turned off? I > would hope that the firmware keeps the gdsc on when it's done > processing things, goes idle, and hands back control to software. > Right now I'm failing to see how the halt bits fail to toggle > assuming that firmware isn't misbehaving and the kernel driver is > power controlling in a coordinated manner with the firmware. What fails is turning ON the clocks after the gdsc is put under hardware control (by fails I mean the halt checks fail to indicate the clock is running, but register accesses etc thereafter suggest the clocks are actually running) The halt checks seem to work only while the gdsc is put in SW enabled state.
On 11/07, Rajendra Nayak wrote: > > > On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote: > > Well I'm also curious which case is failing. Does turning on the > > clocks work after the gdsc is enabled? Does turning off the > > clocks fail because we don't know when the gdsc has turned off? I > > would hope that the firmware keeps the gdsc on when it's done > > processing things, goes idle, and hands back control to software. > > Right now I'm failing to see how the halt bits fail to toggle > > assuming that firmware isn't misbehaving and the kernel driver is > > power controlling in a coordinated manner with the firmware. > > What fails is turning ON the clocks after the gdsc is put under > hardware control (by fails I mean the halt checks fail to indicate > the clock is running, but register accesses etc thereafter suggest > the clocks are actually running) > The halt checks seem to work only while the gdsc is put in SW enabled > state. > Um... that is bad. I don't see how that is possible. It sounds like the clocks are not turning on when we're asking for them to turn on. The register accesses are always working because these subcore clks aren't required for register accesses. Most likely the GDSC for the subdomains is off (even after we thought we enabled it). Let's take a step back. The video hardware has three GDSCs. One for the main logic, and two for each subdomain. We're adding hw control for the two subdomains here. From what I can tell there isn't any hw control for the main domain. I see that there are two registers in the video hardware to control those subdomain hw enable signals that go to the GDSC. The reset value is OFF (not ON like was stated earlier), so it seems that if we switch the subdomain GDSCs on in these patches it will turn on for a short time, and then turn off when we switch into hw mode (by virtue of the way we enable the GDSC). Presumably we can assert these hw signal bits regardless of the subdomain power states, because otherwise we're in a chicken-egg situation for the firmware controlling this stuff. The proper sequence sounds like it should be: 1. Enable GDSC for main domain 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) 3. Write the two registers to assert hw signal for subdomains 4. Enable GDSCs for two subdomains 5. Enable clocks for subdomains (video_subcore{0,1}_clk) I can only guess that we're not doing this. Probably the sequence right now is: 1. Enable GDSC for main domain and both sub-domains 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) 3. Enable clocks for subdomains (video_subcore{0,1}_clk) <clk stuff OFF because hw signal is still deasserted> Sound right? If so, please fix the sequence in the video driver.
Hi Stephen, >> >> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote: >> > Well I'm also curious which case is failing. Does turning on the >> > clocks work after the gdsc is enabled? Does turning off the >> > clocks fail because we don't know when the gdsc has turned off? I >> > would hope that the firmware keeps the gdsc on when it's done >> > processing things, goes idle, and hands back control to software. >> > Right now I'm failing to see how the halt bits fail to toggle >> > assuming that firmware isn't misbehaving and the kernel driver is >> > power controlling in a coordinated manner with the firmware. >> >> What fails is turning ON the clocks after the gdsc is put under >> hardware control (by fails I mean the halt checks fail to indicate >> the clock is running, but register accesses etc thereafter suggest >> the clocks are actually running) >> The halt checks seem to work only while the gdsc is put in SW enabled >> state. >> > >Um... that is bad. I don't see how that is possible. It sounds >like the clocks are not turning on when we're asking for them to >turn on. The register accesses are always working because these >subcore clks aren't required for register accesses. Most likely >the GDSC for the subdomains is off (even after we thought we >enabled it). > >Let's take a step back. The video hardware has three GDSCs. One >for the main logic, and two for each subdomain. We're adding hw >control for the two subdomains here. From what I can tell there >isn't any hw control for the main domain. I see that there are >two registers in the video hardware to control those subdomain hw >enable signals that go to the GDSC. The reset value is OFF (not >ON like was stated earlier), so it seems that if we switch the >subdomain GDSCs on in these patches it will turn on for a short >time, and then turn off when we switch into hw mode (by virtue of >the way we enable the GDSC). Presumably we can assert these hw >signal bits regardless of the subdomain power states, because >otherwise we're in a chicken-egg situation for the firmware >controlling this stuff. > >The proper sequence sounds like it should be: > > 1. Enable GDSC for main domain > 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) > 3. Write the two registers to assert hw signal for subdomains > 4. Enable GDSCs for two subdomains > 5. Enable clocks for subdomains (video_subcore{0,1}_clk) > >I can only guess that we're not doing this. Probably the sequence >right now is: > > 1. Enable GDSC for main domain and both sub-domains > 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) > 3. Enable clocks for subdomains (video_subcore{0,1}_clk) > <clk stuff OFF because hw signal is still deasserted> > >Sound right? If so, please fix the sequence in the video driver. > So the above is the sequence which is actually carried out on the firmware side. The same can be done in host as well. The clocks stuck issue indeed is not there with this. But with the above sequence we need to add a step to do inverse of STEP3 above (ie write the registers to de-assert hw_signal), to keep the subdomains in off, till firmware uses it. So the above sequence helps to avoid masking the halt check, although the host really does not wants to use these clocks, except setting it up for the firmware. Regards, Sricharan -- 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
[].. >> >> The proper sequence sounds like it should be: >> >> 1. Enable GDSC for main domain >> 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) >> 3. Write the two registers to assert hw signal for subdomains >> 4. Enable GDSCs for two subdomains >> 5. Enable clocks for subdomains (video_subcore{0,1}_clk) >> [].. > > So the above is the sequence which is actually carried out on the > firmware side. The same can be done in host as well. By the 'above sequence is done on firmware side', I hope you don;t mean *all* 5 steps. I guess you mean only step 3 is done by firmware? > The clocks stuck issue indeed is not there with this. But with the > above sequence we need to add a step to do inverse of STEP3 > above (ie write the registers to de-assert hw_signal), to keep > the subdomains in off, till firmware uses it. So the above sequence > helps to avoid masking the halt check, although the host really > does not wants to use these clocks, except setting it up for the > firmware.
Hi Rajendra, > >>> >>> The proper sequence sounds like it should be: >>> >>> 1. Enable GDSC for main domain >>> 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) >>> 3. Write the two registers to assert hw signal for subdomains >>> 4. Enable GDSCs for two subdomains >>> 5. Enable clocks for subdomains (video_subcore{0,1}_clk) >>> >[].. > >> >> So the above is the sequence which is actually carried out on the >> firmware side. The same can be done in host as well. > >By the 'above sequence is done on firmware side', I hope you don;t mean *all* 5 steps. >I guess you mean only step 3 is done by firmware? > Yes, only step 3. Regards, Sricharan -- 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
On 11/09, Sricharan wrote: > > So the above is the sequence which is actually carried out on the > firmware side. The same can be done in host as well. > The clocks stuck issue indeed is not there with this. Great! We've finally connected on what the actual problem is. > But with the above sequence we need to add a step to do inverse > of STEP3 above (ie write the registers to de-assert hw_signal), > to keep the subdomains in off, till firmware uses it. So the > above sequence helps to avoid masking the halt check, although > the host really does not wants to use these clocks, except > setting it up for the firmware. > Right, but knowing that the clocks failed to turn on in the first place is much safer than silently ignoring the failure. Otherwise, we could hand over control to the firmware, and the firmware would fail to operate the hardware, and we're stuck with debugging the firmware now. That sounds quite painful to figure out. If we properly toggle the video hw bits in coordination with firmware and turn on/off the clocks with the GDSC ON, then debugging is made simpler. The point is, we don't want to lose robustness by silencing halt checks. The semantics of clk_enable() means the clock is running, and that won't be true here unless we ensure the GDSC is enabled.
Hi Stephen, >> >> So the above is the sequence which is actually carried out on the >> firmware side. The same can be done in host as well. >> The clocks stuck issue indeed is not there with this. > >Great! We've finally connected on what the actual problem is. > >> But with the above sequence we need to add a step to do inverse >> of STEP3 above (ie write the registers to de-assert hw_signal), >> to keep the subdomains in off, till firmware uses it. So the >> above sequence helps to avoid masking the halt check, although >> the host really does not wants to use these clocks, except >> setting it up for the firmware. >> > >Right, but knowing that the clocks failed to turn on in the first >place is much safer than silently ignoring the failure. >Otherwise, we could hand over control to the firmware, and the >firmware would fail to operate the hardware, and we're stuck with >debugging the firmware now. That sounds quite painful to figure >out. > Right, i already debugged this sort of a scenario which was quite paintful sometime back :) >If we properly toggle the video hw bits in coordination with >firmware and turn on/off the clocks with the GDSC ON, then >debugging is made simpler. The point is, we don't want to lose >robustness by silencing halt checks. The semantics of >clk_enable() means the clock is running, and that won't be true >here unless we ensure the GDSC is enabled. > ok, which means with this approach, this patch can be dropped and the other bits needs to be added to the video driver. I will follow that up with Stanimir in his video driver patches. Regards, Sricharan -- 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
Hi Stephen, On 11/09/2016 12:33 AM, 'Stephen Boyd' wrote: > On 11/07, Rajendra Nayak wrote: >> >> >> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote: >>> Well I'm also curious which case is failing. Does turning on the >>> clocks work after the gdsc is enabled? Does turning off the >>> clocks fail because we don't know when the gdsc has turned off? I >>> would hope that the firmware keeps the gdsc on when it's done >>> processing things, goes idle, and hands back control to software. >>> Right now I'm failing to see how the halt bits fail to toggle >>> assuming that firmware isn't misbehaving and the kernel driver is >>> power controlling in a coordinated manner with the firmware. >> >> What fails is turning ON the clocks after the gdsc is put under >> hardware control (by fails I mean the halt checks fail to indicate >> the clock is running, but register accesses etc thereafter suggest >> the clocks are actually running) >> The halt checks seem to work only while the gdsc is put in SW enabled >> state. >> > > Um... that is bad. I don't see how that is possible. It sounds > like the clocks are not turning on when we're asking for them to > turn on. The register accesses are always working because these > subcore clks aren't required for register accesses. Most likely > the GDSC for the subdomains is off (even after we thought we > enabled it). > > Let's take a step back. The video hardware has three GDSCs. One > for the main logic, and two for each subdomain. We're adding hw > control for the two subdomains here. From what I can tell there > isn't any hw control for the main domain. I see that there are > two registers in the video hardware to control those subdomain hw > enable signals that go to the GDSC. The reset value is OFF (not > ON like was stated earlier), so it seems that if we switch the > subdomain GDSCs on in these patches it will turn on for a short > time, and then turn off when we switch into hw mode (by virtue of > the way we enable the GDSC). Presumably we can assert these hw > signal bits regardless of the subdomain power states, because > otherwise we're in a chicken-egg situation for the firmware > controlling this stuff. > > The proper sequence sounds like it should be: > > 1. Enable GDSC for main domain > 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk) > 3. Write the two registers to assert hw signal for subdomains > 4. Enable GDSCs for two subdomains > 5. Enable clocks for subdomains (video_subcore{0,1}_clk) Do you think this sequence 'enable GDSC and the clocks behind it' is valid for MMAGIC GDSCs too? 1. Enable GDSC for MMAGIC_VIDEO 2. Enable clocks (not sure which they are) - msm8996_mmssnoc_axi_rpm_clk, mmss_mmagic_ahb_clk mmss_mmagic_cfg_ahb_clk, mmss_mmagic_maxi_clk, mmagic_video_axi_clk, smmu_video_ahb_clk, smmu_video_axi_clk 3. Continue with the sequence above.
diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c index 41aabe3..8f3f480 100644 --- a/drivers/clk/qcom/mmcc-msm8996.c +++ b/drivers/clk/qcom/mmcc-msm8996.c @@ -1760,6 +1760,7 @@ enum { }; static struct clk_branch video_subcore0_clk = { + .halt_check = BRANCH_HALT_DELAY, .halt_reg = 0x1048, .clkr = { .enable_reg = 0x1048, @@ -1775,6 +1776,7 @@ enum { }; static struct clk_branch video_subcore1_clk = { + .halt_check = BRANCH_HALT_DELAY, .halt_reg = 0x104c, .clkr = { .enable_reg = 0x104c,
With the venus subcore0/1 gdscs(powerdomains) in hw controlled mode, the clock controller does not handle the status bit for the clocks in that domain. So avoid checking for the status bit of those clocks by setting the BRANCH_HALT_DELAY flag. This avoids the WARN_ONs which otherwise occurs when enabling/disabling those clocks. Signed-off-by: Sricharan R <sricharan@codeaurora.org> --- drivers/clk/qcom/mmcc-msm8996.c | 2 ++ 1 file changed, 2 insertions(+)