Re: [PATCH v4 2/2] clk: qcom: gcc-msm8939: Add MSM8939 Generic Clock Controller

[Bryan O'Donoghue]

On 14/05/2020 22:31, Stephen Boyd wrote:
> Quoting Bryan O'Donoghue (2020-05-12 04:50:23)
> > This patch adds support for the MSM8939 GCC. The MSM8939 is based on the
> > MSM8916. MSM8939 is compatible in several ways with MSM8916 but, has
> > additional functional blocks added which require additional PLL sources. In
> > some cases functional blocks from the MSM8916 have different clock sources
> > or different supported frequencies.
> >
> > Cc: Andy Gross <agross@xxxxxxxxxx>
> > Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> > Cc: linux-clk@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
>
> Is this a co-developed-by tag?

Yep. I'm squashing down about 30-some internal patches to this one patch 
here including one or two from Shawn in this set.

I wasn't quite sure what the etiquette on Co-developed was i.e. it 
wasn't something git allowed me to specify with a "git commit -s 
--co-developed="xyz"" so I just retained the SOB.

Looking through git logs I see an example

I'll apply a
Co-developed-by: Shawn Guo <shawn.guo@xxxxxxxxxx>

for v5.

> > +static int gcc_msm8939_probe(struct platform_device *pdev)
> > +{
> > +       int ret;
> > +       struct regmap *regmap;
> > +
> > +       ret = qcom_cc_probe(pdev, &gcc_msm8939_desc);
> > +       if (ret)
> > +               return ret;
> > +
> > +       regmap = dev_get_regmap(&pdev->dev, NULL);
> > +       clk_pll_configure_sr_hpm_lp(&gpll3, regmap, &gpll3_config, true);
> > +       clk_pll_configure_sr_hpm_lp(&gpll4, regmap, &gpll4_config, true);
>
> We should probably configure these before registering the clks. Can you
> do the usual, map registers, configure stuff, and then
> qcom_cc_really_probe()?

I think so. If there was a good reason to configure the plls after the 
registration, I can't recall what that was, maybe the original flow from 
downstream ...

> > +
> > +MODULE_DESCRIPTION("Qualcomm GCC MSM8939 Driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:gcc-msm8939");
>
> The module alias isn't needed right?

Nope g/msm8916/s//msm8939/g - I can zap that.

Thanks for the review.

---
bod