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
