Re: [PATCH v3 3/3] clk: qcom: gcc: Add global clock controller driver for SM8150

[Stephen Boyd <sboyd@kernel.org>]

Quoting Vinod Koul (2019-06-29 06:51:19)
> On 27-06-19, 14:31, Stephen Boyd wrote:
> > Quoting Vinod Koul (2019-06-24 23:31:40)
> > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > index 18bdf34d5e64..076872d195fd 100644
> > > --- a/drivers/clk/qcom/Kconfig
> > > +++ b/drivers/clk/qcom/Kconfig
> > > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> > >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> > >           controller to reset the LPASS subsystem.
> > >  
> > > +config SM_GCC_8150
> > > +       tristate "SM8150 Global Clock Controller"
> > > +       help
> > > +         Support for the global clock controller on SM8150 devices.
> > > +         Say Y if you want to use peripheral devices such as UART,
> > > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> > 
> > Is there eMMC support?
> > 

I guess no?

> > > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > > index f0768fb1f037..4a813b4055d0 100644
> > > --- a/drivers/clk/qcom/Makefile
> > > +++ b/drivers/clk/qcom/Makefile
> > > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> > >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> > >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> > >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> > >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> > >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> > >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > new file mode 100644
> > > index 000000000000..11cd9e19f18d
> > > --- /dev/null
> > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > +static const struct clk_parent_data gcc_parents_5[] = {
> > > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > > +       { .fw_name = "gpll0", .name = "gpll0" },
> > > +       { .fw_name = "gpll7", .name = "gpll7" },
> > > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > 
> > Aren't these gplls all created in this file? They shouldn't be listed in
> > DT so I'm confused why we have .fw_name for them.
> 
> Yes they are and the DT doesnt provide these clock. From what I
> understood from the name conversion to new schema was we should add it
> like above, let me know if I missed something
> 

Yes, you should use the direct clk_hw pointer part of the
clk_parent_data structure instead of having a .fw_name or .name member
in these rows. Any clk inside of the clk controller shouldn't be exposed
into DT just so that we can find it again through string comparisons.