Re: [PATCH v2 5/5] clk: qcom: gcc: Add clock driver for SM8350

[Vinod Koul <vkoul@kernel.org>]

On 10-12-20, 12:43, Stephen Boyd wrote:
> Quoting Vinod Koul (2020-12-07 22:47:02)

> > +config SM_GCC_8350
> > +       tristate "SM8350 Global Clock Controller"
> > +       select QCOM_GDSC
> > +       help
> > +         Support for the global clock controller on SM8350 devices.
> > +         Say Y if you want to use peripheral devices such as UART,
> > +         SPI, I2C, USB, SD/UFS, PCIe etc.
> > +
> > +
> 
> Why double newline?

Will drop

> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> 
> Is this include used?

Will check this and others and drop unused ones

> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> 
> Please add newline here
> 
> > +#include <dt-bindings/clock/qcom,gcc-sm8350.h>
> 
> Please add newline here

Ok to both

> > +static const struct clk_parent_data gcc_parent_data_0[] = {
> > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > +       { .hw = &gcc_gpll0.clkr.hw },
> > +       { .hw = &gcc_gpll0_out_even.clkr.hw },
> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> 
> Is this .fw_name in the binding? Please remove .name everywhere in this
> driver as it shouldn't be necessary.

Ack will drop

> > +static const struct clk_parent_data gcc_parent_data_13[] = {
> > +       { .fw_name = "usb3_phy_wrapper_gcc_usb30_pipe_clk", .name =
> 
> Is this documented in the binding?

Not yet, will update

> > +static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
> > +       .cmd_rcgr = 0x1400c,
> > +       .mnd_width = 8,
> > +       .hid_width = 5,
> > +       .parent_map = gcc_parent_map_6,
> > +       .freq_tbl = ftbl_gcc_sdcc2_apps_clk_src,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "gcc_sdcc2_apps_clk_src",
> > +               .parent_data = gcc_parent_data_6,
> > +               .num_parents = 6,
> > +               .flags = CLK_SET_RATE_PARENT,
> > +               .ops = &clk_rcg2_ops,
> 
> Please use floor ops per Doug's recent patch.

Yes

> > +static struct clk_branch gcc_camera_ahb_clk = {
> > +       .halt_reg = 0x26004,
> > +       .halt_check = BRANCH_HALT_DELAY,
> > +       .hwcg_reg = 0x26004,
> > +       .hwcg_bit = 1,
> > +       .clkr = {
> > +               .enable_reg = 0x26004,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_camera_ahb_clk",
> > +                       .flags = CLK_IS_CRITICAL,
> 
> Why is it critical? Can we just enable it in driver probe and stop
> modeling it as a clk?

it does not have a parent we control, yeah it would make sense to do
that. Tanya do you folks agree ..?

> > +static struct clk_branch gcc_video_axi0_clk = {
> > +       .halt_reg = 0x28010,
> > +       .halt_check = BRANCH_HALT_SKIP,
> 
> Do these need to be halt skip? Is the video axi clk stuff still broken?

I will check on this and update accordingly

> > +static int gcc_sm8350_probe(struct platform_device *pdev)
> > +{
> > +       struct regmap *regmap;
> > +       int ret;
> > +
> > +       regmap = qcom_cc_map(pdev, &gcc_sm8350_desc);
> > +       if (IS_ERR(regmap)) {
> > +               dev_err(&pdev->dev, "Failed to map gcc registers\n");
> > +               return PTR_ERR(regmap);
> > +       }
> > +
> > +       ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* FORCE_MEM_CORE_ON for ufs phy ice core clocks */
> 
> Why?

So I understood that this needs to be set so that ufs clocks can
propagate to ufs mem stuff..

-- 
~Vinod