Re: [PATCH V2 5/7] clk: qcom: Add ipq6018 Global Clock Controller support

[Sricharan R <sricharan@xxxxxxxxxxxxxx>]

Hi Stephen,
  Thanks for the review.

On 12/27/2019 7:03 AM, Stephen Boyd wrote:
> Quoting Sricharan R (2019-12-19 02:41:47)
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 3b33ef1..d0392f1 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -95,6 +95,14 @@ config IPQ_GCC_4019
>>           Say Y if you want to use peripheral devices such as UART, SPI,
>>           i2c, USB, SD/eMMC, etc.
>>  
>> +config IPQ_GCC_6018
>> +       tristate "IPQ6018 Global Clock Controller"
>> +       help
>> +         Support for global clock controller on ipq6018 devices.
>> +         Say Y if you want to use peripheral devices such as UART, SPI,
>> +         i2c, USB, SD/eMMC, etc. Select this for the root clock
>> +         of ipq6018.
> 
> What is the root clock of ipq6018?
> 

	root clock is 'xo'. But i guess this statement is not correct.
	will remove that line.

>> +
>>  config IPQ_GCC_806X
>>         tristate "IPQ806x Global Clock Controller"
>>         help
>> diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
>> new file mode 100644
>> index 0000000..b6f0148
>> --- /dev/null
>> +++ b/drivers/clk/qcom/gcc-ipq6018.c
>> @@ -0,0 +1,4674 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/reset-controller.h>
>> +#include <dt-bindings/clock/qcom,gcc-ipq6018.h>
>> +#include <dt-bindings/reset/qcom,gcc-ipq6018.h>
>> +
>> +#include "common.h"
>> +#include "clk-regmap.h"
>> +#include "clk-pll.h"
>> +#include "clk-rcg.h"
>> +#include "clk-branch.h"
>> +#include "clk-alpha-pll.h"
>> +#include "clk-regmap-divider.h"
>> +#include "clk-regmap-mux.h"
>> +#include "reset.h"
>> +
>> +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
>> +
>> +enum {
>> +       P_XO,
>> +       P_BIAS_PLL,
>> +       P_UNIPHY0_RX,
>> +       P_UNIPHY0_TX,
>> +       P_UNIPHY1_RX,
>> +       P_BIAS_PLL_NSS_NOC,
>> +       P_UNIPHY1_TX,
>> +       P_PCIE20_PHY0_PIPE,
>> +       P_USB3PHY_0_PIPE,
>> +       P_GPLL0,
>> +       P_GPLL0_DIV2,
>> +       P_GPLL2,
>> +       P_GPLL4,
>> +       P_GPLL6,
>> +       P_SLEEP_CLK,
>> +       P_UBI32_PLL,
>> +       P_NSS_CRYPTO_PLL,
>> +       P_PI_SLEEP,
>> +};
>> +
>> +static const struct clk_parent_data gcc_xo_gpll0_gpll0_out_main_div2[] = {
>> +       { .fw_name = "xo", .name = "xo"},
>> +       { .fw_name = "gpll0", .name = "gpll0"},
>> +       { .fw_name = "gpll0_out_main_div2", .name = "gpll0_out_main_div2"},
> 
> Because we aren't migrating this from existing DT to new DT we should be
> able to leave out .name in all these structs. That's the legacy fallback
> mechanism used to migrate DT over to the new way.
> 

	ok will fix it.

>> +};
>> +
>> +static const struct parent_map gcc_xo_gpll0_gpll0_out_main_div2_map[] = {
>> +       { P_XO, 0 },
>> +       { P_GPLL0, 1 },
>> +       { P_GPLL0_DIV2, 4 },
>> +};
>> +
> [...]
>> +
>> +static int gcc_ipq6018_probe(struct platform_device *pdev)
>> +{
>> +       int i, ret;
>> +       struct regmap *regmap;
>> +       struct clk *clk;
>> +       struct device *dev = &pdev->dev;
>> +
>> +       regmap = qcom_cc_map(pdev, &gcc_ipq6018_desc);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       for (i = 0; i < ARRAY_SIZE(gcc_ipq6018_hws); i++) {
>> +               clk = devm_clk_register(&pdev->dev, gcc_ipq6018_hws[i]);
>> +               if (IS_ERR(clk))
>> +                       return PTR_ERR(clk);
>> +       }
>> +
>> +       clk_register_fixed_rate(dev, "pcie20_phy0_pipe_clk", NULL, 0, 250000000);
> 
> Why do we need to register this? Can it come from DT then? Also what if
> it fails? And what if really_probe fails? Then we'll need to undo this
> registration. Ideally this is created somewhere else.
> 

	ok, will move this in to the actual clk list.

>> +
>> +       /* Disable SW_COLLAPSE for USB0 GDSCR */
>> +       regmap_update_bits(regmap, 0x3e078, BIT(0), 0x0);
>> +       /* Enable SW_OVERRIDE for USB0 GDSCR */
>> +       regmap_update_bits(regmap, 0x3e078, BIT(2), BIT(2));
>> +       /* Disable SW_COLLAPSE for USB1 GDSCR */
>> +       regmap_update_bits(regmap, 0x3f078, BIT(0), 0x0);
>> +       /* Enable SW_OVERRIDE for USB1 GDSCR */
>> +       regmap_update_bits(regmap, 0x3f078, BIT(2), BIT(2));
>> +
>> +       /* SW Workaround for UBI Huyara PLL */
>> +       regmap_update_bits(regmap, 0x2501c, BIT(26), BIT(26));
>> +
>> +       clk_alpha_pll_configure(&ubi32_pll_main, regmap, &ubi32_pll_config);
>> +
>> +       clk_alpha_pll_configure(&nss_crypto_pll_main, regmap,
>> +                               &nss_crypto_pll_config);
>> +
>> +       ret = qcom_cc_really_probe(pdev, &gcc_ipq6018_desc, regmap);
>> +
>> +       dev_dbg(&pdev->dev, "Registered ipq6018 clock provider");
> 
> Please remove this and just return the result of really_probe.
> 

 ok, will fix it

Regards,
  Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation