Re: [PATCH] clk: qcom: mmcc8974: add frequency table for gfx3d

[Stephen Boyd]

Quoting Brian Masney (2019-11-04 18:50:32)
> Hi Stephen,
> 
> On Thu, Oct 17, 2019 at 11:13:29AM -0700, Stephen Boyd wrote:
> > Quoting Brian Masney (2019-10-05 18:01:00)
> > > From: Jonathan Marek <jonathan@xxxxxxxx>
> > > 
> > > Add frequency table for the gfx3d clock that's needed in order to
> > > support the GPU upstream on msm8974-based systems.
> > > 
> > > Signed-off-by: Jonathan Marek <jonathan@xxxxxxxx>
> > > Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/clk/qcom/mmcc-msm8974.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> > > index bcb0a397ef91..e70abfe2a792 100644
> > > --- a/drivers/clk/qcom/mmcc-msm8974.c
> > > +++ b/drivers/clk/qcom/mmcc-msm8974.c
> > > @@ -452,10 +452,17 @@ static struct clk_rcg2 mdp_clk_src = {
> > >         },
> > >  };
> > >  
> > > +static struct freq_tbl ftbl_gfx3d_clk_src[] = {
> > > +       F(37500000, P_GPLL0, 16, 0, 0),
> > > +       F(533000000, P_MMPLL0, 1.5, 0, 0),
> > > +       { }
> > > +};
> > 
> > On msm-3.10 kernel the gpu clk seems to be controlled by the RPM[1].
> > What is going on here? This code just looks wrong, but I think it was
> > added as an rcg so that the branch wasn't orphaned and would have some
> > sane frequency. Eventually we planned to parent it to a clk exposed in
> > the RPM clk driver. It's been a while so I'm having a hard time
> > remembering, but I think GPU clk on this device needed to be controlled
> > by RPM so that DDR self refresh wouldn't interact badly with ocmem? Or
> > maybe ocmem needed GPU to be enabled to work? Maybe there is some
> > information in the 3.10 downstream kernel.
> > 
> > [1] https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/arch/arm/mach-msm/clock-rpm-8974.c?h=msm-3.10#n82
> 
> I looked in the MSM 3.4 and 3.10 sources and I can't find that gfx3d
> clock in the mmss (the downstream name for the mmcc that's upstream). I
> even looked through the git history in the 3.10 sources to see if it was
> removed at some point.
> 
> The gfx3d_clk_src was added to mmcc-msm8974.c upstream at the time the
> file was first introduced into the kernel:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d8b212014e69d6b6323773ce6898f224ef4ed0d6
> I haven't been able to find anything else so far where that came from.

Yes. I did that! :) I think it was to make sure there weren't any orphan
clks in the tree, which is why I left the code in place to read the clk
frequency but not change it by omitting a frequency table.

> 
> The GPU works using kmscube and KDE Plasma Mobile with this patch
> applied but won't work without it. As for the status of the GPU working
> upstream for MSM8974: My OCMEM and interconnect patches are now in
> linux-next and are queued for the next merge window. All that's left is
> 1) iommu support, 2) this patch (or whatever it needs to become), and
> 3) add the GPU nodes to device tree for this board.
> 
> Would you be willing to reconsider accepting this patch since its 8974
> specific and is one of the pieces that gets the GPU working upstream?
> 

No? I don't believe the kernel should be controlling this clk through
direct register read/writes. Instead, the GPU frequency should be
controlled through an RPM clk. See oxili_gfx3d_clk_src in the link you
mention above [1] and how it indirects to gfx3d_clk_src via
RPM_MEM_CLK_TYPE and OXILI_ID. In the end, anything that's parented to
this rcg in mmcc should probably just be parented to the RPM clk
instead.