Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191002011555.36571-1-jeffrey.l.hugo@gmail.com>
 <20191002011640.36624-1-jeffrey.l.hugo@gmail.com> <20191017215023.2BFEC20872@mail.kernel.org>
In-Reply-To: <20191017215023.2BFEC20872@mail.kernel.org>
From:   Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Date:   Thu, 17 Oct 2019 17:16:48 -0600
Message-ID: <CAOCk7NqgWkt6BwY75eGS2dbJ7GGk3DqH5NC0VLHUq4fc6WuYog@mail.gmail.com>
Subject: Re: [PATCH v4 1/2] clk: qcom: Add MSM8998 GPU Clock Controller
 (GPUCC) driver
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Marc Gonzalez <marc.w.gonzalez@free.fr>,
        MSM <linux-arm-msm@vger.kernel.org>, linux-clk@vger.kernel.org,
        lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Jeffrey Hugo (2019-10-01 18:16:40)
> > diff --git a/drivers/clk/qcom/gpucc-msm8998.c b/drivers/clk/qcom/gpucc-msm8998.c
> > new file mode 100644
> > index 000000000000..f0ccb4963885
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gpucc-msm8998.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019, Jeffrey Hugo
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/clk.h>
>
> Drop this include please.

Will do.

>
> > +
> > +
> > +static struct clk_rcg2 rbcpr_clk_src = {
> > +       .cmd_rcgr = 0x1030,
> > +       .hid_width = 5,
> > +       .parent_map = gpu_xo_gpll0_map,
> > +       .freq_tbl = ftbl_rbcpr_clk_src,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "rbcpr_clk_src",
> > +               .parent_data = gpu_xo_gpll0,
> > +               .num_parents = 2,
> > +               .ops = &clk_rcg2_ops,
> > +       },
> > +};
> > +
> > +static const struct freq_tbl ftbl_gfx3d_clk_src[] = {
> > +       F(180000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(257000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(342000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(414000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(515000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(596000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(670000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       F(710000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > +       { }
>
> I guess this one doesn't do PLL ping pong? Instead we just reprogram the
> PLL all the time? Can we have rcg2 clk ops that set the rate on the
> parent to be exactly twice as much as the incoming frequency? I thought
> we already had this support in the code. Indeed, it is part of
> _freq_tbl_determine_rate() in clk-rcg.c, but not yet implemented in the
> same function name in clk-rcg2.c! Can you implement it? That way we
> don't need this long frequency table, just this weird one where it looks
> like:
>
>         { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }
>         { }
>
> And then some more logic in the rcg2 ops to allow this possibility for a
> frequency table when CLK_SET_RATE_PARENT is set.

Does not do PLL ping pong.  I'll look at extending the rcg2 ops like
you describe.

>
> > +};
> > +
> > +static struct clk_rcg2 gfx3d_clk_src = {
> > +       .cmd_rcgr = 0x1070,
> > +       .hid_width = 5,
> > +       .parent_map = gpu_xo_gpupll0_map,
> > +       .freq_tbl = ftbl_gfx3d_clk_src,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "gfx3d_clk_src",
> > +               .parent_data = gpu_xo_gpupll0,
> > +               .num_parents = 2,
> > +               .ops = &clk_rcg2_ops,
> > +               .flags = CLK_OPS_PARENT_ENABLE,
>
> Needs CLK_SET_RATE_PARENT presumably?

Ah yeah.  Thanks for catching.

>
> > +       },
> > +};
> > +
> > +static const struct freq_tbl ftbl_rbbmtimer_clk_src[] = {
> > +       F(19200000, P_XO, 1, 0, 0),
> > +       { }
> > +};
> > +
> [...]
> > +
> > +static const struct qcom_cc_desc gpucc_msm8998_desc = {
> > +       .config = &gpucc_msm8998_regmap_config,
> > +       .clks = gpucc_msm8998_clocks,
> > +       .num_clks = ARRAY_SIZE(gpucc_msm8998_clocks),
> > +       .resets = gpucc_msm8998_resets,
> > +       .num_resets = ARRAY_SIZE(gpucc_msm8998_resets),
> > +       .gdscs = gpucc_msm8998_gdscs,
> > +       .num_gdscs = ARRAY_SIZE(gpucc_msm8998_gdscs),
> > +};
> > +
> > +static const struct of_device_id gpucc_msm8998_match_table[] = {
> > +       { .compatible = "qcom,gpucc-msm8998" },
>
> The compatible is different. In the merged binding it is
> qcom,msm8998-gpucc. Either fix this or fix the binding please.

This is wrong.  Will fix.