Quoting Vinod (2018-10-02 23:21:03)
> Hi Stephen,
>
> Thanks for the comments,
>
> On 01-10-18, 10:19, Stephen Boyd wrote:
> > Quoting Vinod Koul (2018-09-21 11:59:36)
> > > From: Shefali Jain <shefj...@codeaurora.org>
> > >
> > > Add the clocks supported in global clock controller which clock the
> > > peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks
> > > to the clock framework for the clients to be able to request for them.
> > >
> > > Signed-off-by: Shefali Jain <shefj...@codeaurora.org>
> > > Signed-off-by: Taniya Das <t...@codeaurora.org>
> > > Co-developed-by: Taniya Das <t...@codeaurora.org>
> > > Signed-off-by: Anu Ramanathan <a...@codeaurora.org>
> > > [rebase and tidyup for upstream]
> >
> > Who did the tidying?
>
> both of us :)
OK, please add the username of both people per the kernel sign off
standards.
>
> > > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> > > Signed-off-by: Vinod Koul <vk...@kernel.org>
> > > ---
> > > - reg : shall contain base register location and length
> > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > index 064768699fe7..529d84cc7503 100644
> > > --- a/drivers/clk/qcom/Kconfig
> > > +++ b/drivers/clk/qcom/Kconfig
> > > @@ -235,6 +235,14 @@ config MSM_GCC_8998
> > > Say Y if you want to use peripheral devices such as UART, SPI,
> > > i2c, USB, UFS, SD/eMMC, PCIe, etc.
> > >
> > > +config QCS_GCC_404
> > > + tristate "QCS404 Global Clock Controller"
> > > + depends on COMMON_CLK_QCOM
> > > + help
> > > + Support for the global clock controller on QCS404 devices.
> > > + Say Y if you want to use peripheral devices such as UART, SPI,
> > > I2C,
> > > + USB, SD/eMMC, PCIe, etc.
> >
> > It seems to include multimedia display clks and ethernet? Maybe include
> > those too.
>
> Sure will add
>
> > > +#include <linux/kernel.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/clk.h>
> >
> > Please don't include this.
>
> OK will check if this is required, any reason for not including this?
So we can easily split clk consumers and clk providers.
>
> > > +static struct clk_branch gcc_pwm1_xo512_clk = {
> > > + .halt_reg = 0x49004,
> > > + .halt_check = BRANCH_HALT,
> > > + .clkr = {
> > > + .enable_reg = 0x49004,
> > > + .enable_mask = BIT(0),
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "gcc_pwm1_xo512_clk",
> > > + .ops = &clk_branch2_ops,
> >
> > Do these pwm clks have a parent clk of the XO?
>
> Yes they do
Cool, we should add them or add a comment explaining why they don't have
parents listed here.
>
> > > + [GCC_USB_HS_PHY_CFG_AHB_CLK] = &gcc_usb_hs_phy_cfg_ahb_clk.clkr,
> > > + [GCC_USB_HS_SYSTEM_CLK] = &gcc_usb_hs_system_clk.clkr,
> > > + [GFX3D_CLK_SRC] = &gfx3d_clk_src.clkr,
> > > + [GP1_CLK_SRC] = &gp1_clk_src.clkr,
> >
> > Why are some of these missing GCC_ prefix?
>
> will add..
Thanks!
>
> > > +static int gcc_qcs404_probe(struct platform_device *pdev)
> > > +{
> > > + struct regmap *regmap;
> > > + int ret;
> > > +
> > > + ret = qcom_cc_register_board_clk(&pdev->dev,
> > > + "xo_board", "cxo", 19200000);
> >
> > You shouldn't need to do this. This function is for transitioning DT
> > that doesn't have the board clk in DT to something the driver wants to
> > use, in this case "cxo". So you can either register a fixed factor 1/1
> > clk to do the translation between board and cxo names, or use xo_board
> > as the parent of things that can take crystal.
>
> Okay will modify this. If I go about using xo_board as parent, I would
> need to register that right? FWIW I see the same thing done in gcc-msm8916
Yes it would be similar to 8916 or 8996/845.
>
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + regmap = qcom_cc_map(pdev, &gcc_qcs404_desc);
> > > + if (IS_ERR(regmap))
> > > + return PTR_ERR(regmap);
> > > +
> > > + clk_alpha_pll_configure(&gpll3_out_main, regmap, &gpll3_config);
> > > + clk_set_rate(apss_ahb_clk_src.clkr.hw.clk, 19200000);
> >
> > use assigned clock rates from DT please.
>
> ok
>
> > > + clk_prepare_enable(apss_ahb_clk_src.clkr.hw.clk);
> > > + clk_prepare_enable(gpll0_ao_out_main.clkr.hw.clk);
> >
> > And these should be marked as critical clocks.
>
> Okay and how do we go about doing that?
You mark the clk flags with CLK_IS_CRITICAL.
