On Fri, Jun 7, 2019 at 5:18 PM Stephen Boyd [off-list ref] wrote:

Quoting Jeffrey Hugo (2019-06-07 14:31:13)

quoted

On Fri, Jun 7, 2019 at 2:38 PM Stephen Boyd [off-list ref] wrote:

quoted

Quoting Jeffrey Hugo (2019-05-21 07:52:28)

quoted

On 5/21/2019 8:44 AM, Jeffrey Hugo wrote:

quoted

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.

quoted

quoted

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.

quoted

-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.