Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Govind Singh <govinds@codeaurora.org>,
	bjorn.andersson@linaro.org, linux-remoteproc@vger.kernel.org
Cc: linux-clk@vger.kernel.org, sricharan@codeaurora.org,
	sibis@codeaurora.org, linux-arm-msm@vger.kernel.org,
	andy.gross@linaro.org, david.brown@linaro.org,
	linux-soc@vger.kernel.org, devicetree@vger.kernel.org,
	Govind Singh <govinds@codeaurora.org>
Subject: Re: [PATCH v3 2/7] clk: qcom: Add WCSS Q6DSP clock controller for QCS404
Date: Mon, 17 Dec 2018 10:52:04 -0800
Message-ID: <154507272487.19322.760504004108754132@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20181215103557.2748-3-govinds@codeaurora.org>

Quoting Govind Singh (2018-12-15 02:35:52)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 9fe28b9ceba8..84acc7718691 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -222,6 +222,15 @@ config QCS_GCC_404
>           Say Y if you want to use multimedia devices or peripheral
>           devices such as UART, SPI, I2C, USB, SD/eMMC, PCIe etc.
>  
> +config QCS_WCSSCC_404
> +       tristate "QCS404 WCSS Clock Controller"
> +       depends on COMMON_CLK_QCOM

This is going away, so you can drop this depends on statement soon.

> +       select QCS_GCC_404
> +       help
> +         Support for the WCSS clock controller on QCS404 devices.
> +         Say Y if you want to use the WCSS branch clocks of the WCSS clock
> +         controller to reset the WCSS subsystem.
> +
>  config SDM_GCC_845
>         tristate "SDM845 Global Clock Controller"
>         select QCOM_GDSC
> diff --git a/drivers/clk/qcom/wcsscc-qcs404.c b/drivers/clk/qcom/wcsscc-qcs404.c
> new file mode 100644
> index 000000000000..bd694ef1b6ac
> --- /dev/null
> +++ b/drivers/clk/qcom/wcsscc-qcs404.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>

Is this used?

> +#include <linux/clk.h>

Is this used?

> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

Is this used?

> +
> +#include <dt-bindings/clock/qcom,wcss-qcs404.h>
> +#include <linux/reset-controller.h>

Cool, but where are the resets? Also, please put <linux/*.h> before any
dt-bindings includes.

> +
> +#include "clk-regmap.h"
> +#include "clk-branch.h"
> +#include "common.h"
> +#include "reset.h"
> +
> +/* Q6SSTOP clocks. These clocks are voted
> + * during by remoteproc client when loaded from

This sentence doesn't parse easily. "during by"?

> + * user space, soc hang is seen when CCF turns

SoC? Or just system hang?

> + * off unused clocks. As a temp solution use

Temporary?

> + * CLK_IGNORE_UNUSED flags which prevent these
> + * clocks from being gated during bootup.

Ok.. but userspace is after CLK_IGNORE_UNUSED would process these clks?
So we're keeping them on from the bootloader why? Something is using
these clks during that operation but after that point they need to be
turned off?

> + */
> +
> +static int wcss_clocks_qcs404_probe(struct platform_device *pdev, int index,
> +                                   const struct qcom_cc_desc *desc)
> +{
> +       struct regmap *regmap;
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return -ENOMEM;
> +
> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       return qcom_cc_really_probe(pdev, desc, regmap);
> +}

You're the second user of this "probe on reg region" logic. Please
extract it out of the lpasscc driver and put it into common.c so it can
be reused by the two drivers.

> +
> +static int wcss_cc_qcs404_probe(struct platform_device *pdev)
> +{
> +       const struct qcom_cc_desc *desc;
> +       int ret;
> +
> +       wcss_regmap_config.name = "wcss_q6sstop";
> +       desc = &wcss_q6sstop_qcs404_desc;
> +
> +       ret = wcss_clocks_qcs404_probe(pdev, 0, desc);
> +       if (ret)
> +               return ret;
> +
> +       wcss_regmap_config.name = "wcnss_tcsr";
> +       desc = &wcnss_tcsr_qcs404_desc;
> +
> +       ret = wcss_clocks_qcs404_probe(pdev, 1, desc);
> +       if (ret)
> +               return ret;
> +
> +       wcss_regmap_config.name = "wcss_qdsp6ss";
> +       desc = &wcnss_qdsp6ss_qcs404_desc;
> +
> +       return wcss_clocks_qcs404_probe(pdev, 2, desc);
> +}
> +
> +static struct platform_driver wcss_cc_qcs404_driver = {
> +       .probe          = wcss_cc_qcs404_probe,
> +       .driver         = {
> +               .name   = "qcs404-wcsscc",
> +               .of_match_table = wcss_cc_qcs404_match_table,
> +       },
> +};
> +
> +static int __init wcss_cc_qcs404_init(void)
> +{
> +       return platform_driver_register(&wcss_cc_qcs404_driver);
> +}
> +subsys_initcall(wcss_cc_qcs404_init);

Where is the driver removal exit function?

> +
> +MODULE_LICENSE("GPL v2");

MODULE_DESCRIPTION?


  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-15 10:35 [PATCH v3 0/7] Add non PAS wcss Q6 support " Govind Singh
2018-12-15 10:35 ` [PATCH v3 1/7] dt-bindings: clock: qcom: Introduce QCOM WCSS Q6DSP clock bindings Govind Singh
2018-12-17 19:33   ` Stephen Boyd
2019-02-02 15:35     ` Govind Singh
2018-12-15 10:35 ` [PATCH v3 2/7] clk: qcom: Add WCSS Q6DSP clock controller for QCS404 Govind Singh
2018-12-17 18:52   ` Stephen Boyd [this message]
2019-02-02 15:45     ` Govind Singh
2018-12-15 10:35 ` [PATCH v3 3/7] dt-bindings: clock: qcom: Add QCOM WCSS GCC clock bindings Govind Singh
2018-12-17 19:34   ` Stephen Boyd
2019-02-02 15:33     ` Govind Singh
2018-12-15 10:35 ` [PATCH v3 4/7] clk: qcom: Add WCSS gcc clock control for QCS404 Govind Singh
2018-12-15 17:56   ` Bjorn Andersson
2019-02-02 15:32     ` Govind Singh
2018-12-17 18:53   ` Stephen Boyd
2018-12-15 10:35 ` [PATCH v3 5/7] remoteproc: qcom: wcss: populate hardcoded param using driver data Govind Singh
2018-12-17 18:55   ` Stephen Boyd
2018-12-15 10:35 ` [PATCH v3 6/7] remoteproc: qcom: wcss: Add non pas wcss Q6 support for QCS404 Govind Singh
2018-12-17 19:00   ` Stephen Boyd
2018-12-17 20:48   ` Rob Herring
2018-12-15 10:35 ` [PATCH v3 7/7] remoteproc: qcom: wcss: explicitly request exclusive reset control Govind Singh

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=154507272487.19322.760504004108754132@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=govinds@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=sibis@codeaurora.org \
    --cc=sricharan@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox