| Message ID | 1558449843-19971-1-git-send-email-jhugo@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | MSM8998 Multimedia Clock Controller | expand |
On 5/21/2019 8:44 AM, Jeffrey Hugo wrote: > The multimedia clock controller (mmcc) is the main clock controller for > the multimedia subsystem and is required to enable things like display and > camera. Stephen, I think this series is good to go, and I have display/gpu stuff I'm polishing that will depend on this. Would you kindly pickup patches 1, 3, 4, and 5 for 5.3? I can work with Bjorn to pick up patches 2 and 6. > > v4: > -fix makefile to use correct config item > -pick up tags > -fix ordering of clocks and clock-names in dt > -drop MODULE_ALIAS > -wait for xo in mmcc since that was found to be useful in some debug configs > > v3: > -Rebase onto linux-next to get the final version of the clk parent rewrite > series > -Moved the bindings header to the bindings patch per Rob > -Made xo manditory for GCC to work around the lack of clk orphan probe defer > to avoid the uart console glitch > > v2: > -Rebased on the "Rewrite clk parent handling" series and updated to the clk init > mechanisms introduced there. > -Marked XO clk as CLK_IGNORE_UNUSED to avoid the concern about the XO going away > "incorrectly" during late init > -Corrected the name of the XO clock to "xo" > -Dropped the fake XO clock in GCC to prevent a namespace conflict > -Fully enumerated the external clocks (DSI PLLs, etc) in the DT binding > -Cleaned up the weird newlines in the added DT node > -Added DT header file to msm8998 DT for future clients > > Jeffrey Hugo (6): > dt-bindings: clock: Document external clocks for MSM8998 gcc > arm64: dts: msm8998: Add xo clock to gcc node > clk: qcom: smd: Add XO clock for MSM8998 > dt-bindings: clock: Add support for the MSM8998 mmcc > clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver > arm64: dts: qcom: msm8998: Add mmcc node > > .../devicetree/bindings/clock/qcom,gcc.txt | 10 + > .../devicetree/bindings/clock/qcom,mmcc.txt | 21 + > arch/arm64/boot/dts/qcom/msm8998.dtsi | 16 + > drivers/clk/qcom/Kconfig | 9 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/clk-smd-rpm.c | 24 +- > drivers/clk/qcom/gcc-msm8998.c | 29 +- > drivers/clk/qcom/mmcc-msm8998.c | 2915 +++++++++++++++++ > include/dt-bindings/clock/qcom,mmcc-msm8998.h | 210 ++ > 9 files changed, 3214 insertions(+), 21 deletions(-) > create mode 100644 drivers/clk/qcom/mmcc-msm8998.c > create mode 100644 include/dt-bindings/clock/qcom,mmcc-msm8998.h >
Quoting Jeffrey Hugo (2019-05-21 07:52:28) > On 5/21/2019 8:44 AM, Jeffrey Hugo wrote: > > The multimedia clock controller (mmcc) is the main clock controller for > > the multimedia subsystem and is required to enable things like display and > > camera. > > Stephen, I think this series is good to go, and I have display/gpu stuff > I'm polishing that will depend on this. Would you kindly pickup patches > 1, 3, 4, and 5 for 5.3? I can work with Bjorn to pick up patches 2 and 6. > If I apply patch 3 won't it break boot until patch 2 is also in the tree? That seems to imply that I'll break bisection, and we have kernelci boot testing clk-next so this will probably set off alarms somewhere. I thought we had some code that got removed that was going to make the transition "seamless" in the sense that it would search the tree for an RPM clk controller and then not add the XO fixed factor clk somehow. See commit 54823af9cd52 ("clk: qcom: Always add factor clock for xo clocks") for the code that we removed. So ideally we do something like this too, but now we search for a property on the calling node to see if the XO clk is there?
On Fri, Jun 7, 2019 at 2:38 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Jeffrey Hugo (2019-05-21 07:52:28) > > On 5/21/2019 8:44 AM, Jeffrey Hugo wrote: > > > The multimedia clock controller (mmcc) is the main clock controller for > > > the multimedia subsystem and is required to enable things like display and > > > camera. > > > > Stephen, I think this series is good to go, and I have display/gpu stuff > > I'm polishing that will depend on this. Would you kindly pickup patches > > 1, 3, 4, and 5 for 5.3? I can work with Bjorn to pick up patches 2 and 6. > > > > If I apply patch 3 won't it break boot until patch 2 is also in the > tree? That seems to imply that I'll break bisection, and we have > kernelci boot testing clk-next so this will probably set off alarms > somewhere. Yes, it'll break boot. Maybe you and Bjorn can make a deal? (more below) Doesn't look like kernelci is running tests on 8998 anymore, so maybe no one will complain? As far as I am aware, Marc, Lee, Bjorn, and I are the only ones whom care about 8998 presently, and I think we are all good with a temporary breakage in order to get this basic functionality in since the platform isn't really well supported yet. > > I thought we had some code that got removed that was going to make the > transition "seamless" in the sense that it would search the tree for an > RPM clk controller and then not add the XO fixed factor clk somehow. > See commit 54823af9cd52 ("clk: qcom: Always add factor clock for xo > clocks") for the code that we removed. So ideally we do something like > this too, but now we search for a property on the calling node to see if > the XO clk is there? > Trying to remember back a bit. I don't think its possible. Maybe I'm wrong. I didn't see a solution to the below: How does GCC know the following? -RPMCC is compiled in the build (I guess this can be assumed) -RPMCC has probed -RPMCC is not and will not be providing XO
Quoting Jeffrey Hugo (2019-06-07 14:31:13) > On Fri, Jun 7, 2019 at 2:38 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Jeffrey Hugo (2019-05-21 07:52:28) > > > On 5/21/2019 8:44 AM, Jeffrey Hugo wrote: > > > > The multimedia clock controller (mmcc) is the main clock controller for > > > > the multimedia subsystem and is required to enable things like display and > > > > camera. > > > > > > Stephen, I think this series is good to go, and I have display/gpu stuff > > > I'm polishing that will depend on this. Would you kindly pickup patches > > > 1, 3, 4, and 5 for 5.3? I can work with Bjorn to pick up patches 2 and 6. > > > > > > > If I apply patch 3 won't it break boot until patch 2 is also in the > > tree? That seems to imply that I'll break bisection, and we have > > kernelci boot testing clk-next so this will probably set off alarms > > somewhere. > > Yes, it'll break boot. Maybe you and Bjorn can make a deal? (more below) > > Doesn't look like kernelci is running tests on 8998 anymore, so maybe > no one will complain? As far as I am aware, Marc, Lee, Bjorn, and I > are the only ones whom care about 8998 presently, and I think we are > all good with a temporary breakage in order to get this basic > functionality in since the platform isn't really well supported yet. Ok. > > > > > I thought we had some code that got removed that was going to make the > > transition "seamless" in the sense that it would search the tree for an > > RPM clk controller and then not add the XO fixed factor clk somehow. > > See commit 54823af9cd52 ("clk: qcom: Always add factor clock for xo > > clocks") for the code that we removed. So ideally we do something like > > this too, but now we search for a property on the calling node to see if > > the XO clk is there? > > > > Trying to remember back a bit. > > I don't think its possible. Maybe I'm wrong. I didn't see a solution > to the below: > > How does GCC know the following? > -RPMCC is compiled in the build (I guess this can be assumed) This is the IS_ENABLED part. > -RPMCC has probed > -RPMCC is not and will not be providing XO Presumably if it's enabled then it will be providing XO at some point in the future. I'm not suggesting the probe defer logic is removed, just that we don't get into a state where clk tree has merged all the patches for clk driver side and that then relies on DT to provide the clk but it doesn't do that. So the idea is to check if RPM is compiled in and also check the GCC DT node for the clocks property having the xo clk there. Then we assume that we have the clk patches in place for the RPM clk driver to provide that clk and we skip inserting the fake clk that RPM is going to provide. This is also a "general" solution to GCC not depending on or selecting the RPM clk driver. It may be better to just have a select statement in GCC Kconfig so that we can't enable the GCC driver without also enabling the RPM driver if it's an essential dependency to the clk tree working. But if we do this design then we can make the clk tree keep working regardless of RPM being there or not, which may be a good thing.
On Fri, Jun 7, 2019 at 5:18 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Jeffrey Hugo (2019-06-07 14:31:13) > > On Fri, Jun 7, 2019 at 2:38 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Jeffrey Hugo (2019-05-21 07:52:28) > > > > On 5/21/2019 8:44 AM, Jeffrey Hugo wrote: > > > > > The multimedia clock controller (mmcc) is the main clock controller for > > > > > the multimedia subsystem and is required to enable things like display and > > > > > camera. > > > > > > > > Stephen, I think this series is good to go, and I have display/gpu stuff > > > > I'm polishing that will depend on this. Would you kindly pickup patches > > > > 1, 3, 4, and 5 for 5.3? I can work with Bjorn to pick up patches 2 and 6. > > > > > > > > > > If I apply patch 3 won't it break boot until patch 2 is also in the > > > tree? That seems to imply that I'll break bisection, and we have > > > kernelci boot testing clk-next so this will probably set off alarms > > > somewhere. > > > > Yes, it'll break boot. Maybe you and Bjorn can make a deal? (more below) > > > > Doesn't look like kernelci is running tests on 8998 anymore, so maybe > > no one will complain? As far as I am aware, Marc, Lee, Bjorn, and I > > are the only ones whom care about 8998 presently, and I think we are > > all good with a temporary breakage in order to get this basic > > functionality in since the platform isn't really well supported yet. > > Ok. > > > > > > > > > I thought we had some code that got removed that was going to make the > > > transition "seamless" in the sense that it would search the tree for an > > > RPM clk controller and then not add the XO fixed factor clk somehow. > > > See commit 54823af9cd52 ("clk: qcom: Always add factor clock for xo > > > clocks") for the code that we removed. So ideally we do something like > > > this too, but now we search for a property on the calling node to see if > > > the XO clk is there? > > > > > > > Trying to remember back a bit. > > > > I don't think its possible. Maybe I'm wrong. I didn't see a solution > > to the below: > > > > How does GCC know the following? > > -RPMCC is compiled in the build (I guess this can be assumed) > > This is the IS_ENABLED part. Eh, this would assume that someone hasn't compiled RPCC out of tree, but this is probably me being pedantic. > > > -RPMCC has probed > > -RPMCC is not and will not be providing XO > > Presumably if it's enabled then it will be providing XO at some point in > the future. I'm not suggesting the probe defer logic is removed, just > that we don't get into a state where clk tree has merged all the patches > for clk driver side and that then relies on DT to provide the clk but it > doesn't do that. > > So the idea is to check if RPM is compiled in and also check the GCC DT > node for the clocks property having the xo clk there. Then we assume > that we have the clk patches in place for the RPM clk driver to provide > that clk and we skip inserting the fake clk that RPM is going to > provide. So, I thought about this, and I don't think it works. This appears to be a solution for if RPM is not present. If, in patch 3, I check for the clocks property in the gcc DT and don't see it, and I add the fake XO as a result, I'll end up with a namespace conflict with either GCC or RPMCC failing to register XO since both will attempt it. The issue is do we have RPM providing XO (ie patch 3) without a DT routing it to GCC (ie patch 2). Since we have patch 3, we know RPM is going to provide XO. We just might not have a handle to it to do the probe_defer logic. I think the solution is check the DT for the clocks property, if present use that to attempt to grab XO. Otherwise, attempt to grab XO from the system map. I'll try this in the next rev. > > This is also a "general" solution to GCC not depending on or selecting > the RPM clk driver. It may be better to just have a select statement in > GCC Kconfig so that we can't enable the GCC driver without also enabling > the RPM driver if it's an essential dependency to the clk tree working. > But if we do this design then we can make the clk tree keep working > regardless of RPM being there or not, which may be a good thing. > I'm not sure attempting to sidestep the RPM dependency is worthwhile. Its not just GCC, but every clock controller that depends on XO, and thus implicitly RPM. I suspect any real system is going to have to have the RPM, just by the nature of what the RPM controls that cannot be managed elsewhere, such as BIMC. At some point, a system is going to want to ramp up the memory bus and DDR speeds to something more than the bare minimum. Putting in a bunch of code to try to avoid depending on the RPM seems like effort not well spent as its unlikely such code is going to be active.