Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1277981 > unrolled thread

[PATCH 0/6] Add support for MSM8996 GDSCs

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] Add support for MSM8996 GDSCs Rajendra Nayak <rnayak@codeaurora.org> - 2015-11-26 07:00 +0100
    [PATCH 3/6] clk: qcom: gdsc: Add GDSCs in msm8996 GCC Rajendra Nayak <rnayak@codeaurora.org> - 2015-11-26 07:00 +0100
    [PATCH 1/6] clk: qcom: gdsc: Add support for hierarchical power domains Rajendra Nayak <rnayak@codeaurora.org> - 2015-11-26 07:00 +0100
    [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller Rajendra Nayak <rnayak@codeaurora.org> - 2015-11-26 07:00 +0100
      Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw  controller Stephen Boyd <sboyd@codeaurora.org> - 2015-12-01 03:30 +0100
        Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw  controller Rajendra Nayak <rnayak@codeaurora.org> - 2015-12-01 04:10 +0100
          Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw  controller Stephen Boyd <sboyd@codeaurora.org> - 2015-12-01 08:30 +0100
            Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw  controller "Rajendra Nayak" <rnayak@codeaurora.org> - 2015-12-01 17:10 +0100
    [PATCH 6/6] clk: qcom: mmcc8974: Use gdscs .parent and remove genpd calls Rajendra Nayak <rnayak@codeaurora.org> - 2015-11-26 07:00 +0100

#1277981 — [PATCH 0/6] Add support for MSM8996 GDSCs

This series adds support for GDSCs' which are part of gcc and mmcc
in QCOM msm8996 SoC. Series applies on top of the patches[1] which
adds support for MSM8996 clocks.

There are many more cases of gdscs within gdscs (hierarchical power
domains) in msm8996 (msm8974 has one such instance which is supported
upstream) and hence the series adds support within gdsc driver to
handle this.

Also msm8996 has gdscs with gds hw controllers within, for ensuring
slave device idleness before gdscs are powered off, hence the series
also adds support for gdscs with gds hw controllers.

ToDo: Series does not yet add support for gpu gdscs which are part
of mmcc.

Tested on apq8096 dragonboards to make sure all modelled gdscs can
be turned on/off successfully.

[1] https://lkml.org/lkml/2015/11/17/949 

Rajendra Nayak (6):
  clk: qcom: gdsc: Add support for hierarchical power domains
  clk: qcom: gdsc: Add support for gdscs with gds hw controller
  clk: qcom: gdsc: Add GDSCs in msm8996 GCC
  clk: qcom: gdsc: Add mmcc gdscs for msm8996 family
  clk: qcom: gdsc: Do not check for disabled status on votable gdscs
  clk: qcom: mmcc8974: Use gdscs .parent and remove genpd calls

 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   2 +
 drivers/clk/qcom/common.c                     |  14 ++-
 drivers/clk/qcom/gcc-msm8996.c                |  92 +++++++++++++++
 drivers/clk/qcom/gdsc.c                       |  69 ++++++++---
 drivers/clk/qcom/gdsc.h                       |  34 ++++--
 drivers/clk/qcom/mmcc-msm8974.c               |  14 +--
 drivers/clk/qcom/mmcc-msm8996.c               | 157 ++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8996.h  |  11 ++
 include/dt-bindings/clock/qcom,mmcc-msm8996.h |  17 +++
 9 files changed, 363 insertions(+), 47 deletions(-)

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1277983 — [PATCH 3/6] clk: qcom: gdsc: Add GDSCs in msm8996 GCC

Add all data for the GDSCs which are part of msm8996 GCC block

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi        |  1 +
 drivers/clk/qcom/gcc-msm8996.c               | 88 ++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8996.h | 11 ++++
 3 files changed, 100 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 2c2736d..31e7bd9 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -147,6 +147,7 @@
 			compatible = "qcom,gcc-msm8996";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
+			#power-domain-cells = <1>;
 			reg = <0x300000 0x90000>;
 		};
 
diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 16d7c32..c5bce5f 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -30,6 +30,7 @@
 #include "clk-rcg.h"
 #include "clk-branch.h"
 #include "reset.h"
+#include "gdsc.h"
 
 #define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
 
@@ -3059,6 +3060,79 @@ static struct clk_hw *gcc_msm8996_hws[] = {
 	&ufs_ice_core_postdiv_clk_src.hw,
 };
 
+static struct gdsc aggre0_noc_gdsc = {
+	.gdscr = 0x81004,
+	.gds_hw_ctrl = 0x81028,
+	.pd = {
+		.name = "aggre0_noc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc hlos1_vote_aggre0_noc_gdsc = {
+	.gdscr = 0x7d024,
+	.pd = {
+		.name = "hlos1_vote_aggre0_noc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc hlos1_vote_lpass_adsp_gdsc = {
+	.gdscr = 0x7d034,
+	.pd = {
+		.name = "hlos1_vote_lpass_adsp",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc hlos1_vote_lpass_core_gdsc = {
+	.gdscr = 0x7d038,
+	.pd = {
+		.name = "hlos1_vote_lpass_core",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc usb30_gdsc = {
+	.gdscr = 0xf004,
+	.pd = {
+		.name = "usb30",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc pcie0_gdsc = {
+	.gdscr = 0x6b004,
+	.pd = {
+		.name = "pcie0",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc pcie1_gdsc = {
+	.gdscr = 0x6d004,
+	.pd = {
+		.name = "pcie1",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc pcie2_gdsc = {
+	.gdscr = 0x6e004,
+	.pd = {
+		.name = "pcie2",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc ufs_gdsc = {
+	.gdscr = 0x75004,
+	.pd = {
+		.name = "ufs",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
 static struct clk_regmap *gcc_msm8996_clocks[] = {
 	[GPLL0_EARLY] = &gpll0_early.clkr,
 	[GPLL0] = &gpll0.clkr,
@@ -3245,6 +3319,18 @@ static struct clk_regmap *gcc_msm8996_clocks[] = {
 	[GCC_RX1_USB2_CLKREF_CLK] = &gcc_rx1_usb2_clkref_clk.clkr,
 };
 
+static struct gdsc *gcc_msm8996_gdscs[] = {
+	[AGGRE0_NOC_GDSC] = &aggre0_noc_gdsc,
+	[HLOS1_VOTE_AGGRE0_NOC_GDSC] = &hlos1_vote_aggre0_noc_gdsc,
+	[HLOS1_VOTE_LPASS_ADSP_GDSC] = &hlos1_vote_lpass_adsp_gdsc,
+	[HLOS1_VOTE_LPASS_CORE_GDSC] = &hlos1_vote_lpass_core_gdsc,
+	[USB30_GDSC] = &usb30_gdsc,
+	[PCIE0_GDSC] = &pcie0_gdsc,
+	[PCIE1_GDSC] = &pcie1_gdsc,
+	[PCIE2_GDSC] = &pcie2_gdsc,
+	[UFS_GDSC] = &ufs_gdsc,
+};
+
 static const struct qcom_reset_map gcc_msm8996_resets[] = {
 	[GCC_SYSTEM_NOC_BCR] = { 0x4000 },
 	[GCC_CONFIG_NOC_BCR] = { 0x5000 },
@@ -3363,6 +3449,8 @@ static const struct qcom_cc_desc gcc_msm8996_desc = {
 	.num_clks = ARRAY_SIZE(gcc_msm8996_clocks),
 	.resets = gcc_msm8996_resets,
 	.num_resets = ARRAY_SIZE(gcc_msm8996_resets),
+	.gdscs = gcc_msm8996_gdscs,
+	.num_gdscs = ARRAY_SIZE(gcc_msm8996_gdscs),
 };
 
 static const struct of_device_id gcc_msm8996_match_table[] = {
diff --git a/include/dt-bindings/clock/qcom,gcc-msm8996.h b/include/dt-bindings/clock/qcom,gcc-msm8996.h
index 888e75c..6f814db 100644
--- a/include/dt-bindings/clock/qcom,gcc-msm8996.h
+++ b/include/dt-bindings/clock/qcom,gcc-msm8996.h
@@ -336,4 +336,15 @@
 #define GCC_MSS_Q6_BCR						99
 #define GCC_QREFS_VBG_CAL_BCR					100
 
+/* Indexes for GDSCs */
+#define AGGRE0_NOC_GDSC			0
+#define HLOS1_VOTE_AGGRE0_NOC_GDSC	1
+#define HLOS1_VOTE_LPASS_ADSP_GDSC	2
+#define HLOS1_VOTE_LPASS_CORE_GDSC	3
+#define USB30_GDSC			4
+#define PCIE0_GDSC			5
+#define PCIE1_GDSC			6
+#define PCIE2_GDSC			7
+#define UFS_GDSC			8
+
 #endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277985 — [PATCH 1/6] clk: qcom: gdsc: Add support for hierarchical power domains

Some qcom SoCs' can have hierarchical power domains. Let the gdsc structs
specify the parents (if any) and the driver add genpd subdomains for them.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/common.c | 14 +++++++++-----
 drivers/clk/qcom/gdsc.c   | 27 +++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h   | 17 ++++++++++++-----
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 8fa4772..cf100f9 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -98,6 +98,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	struct clk **clks;
 	struct qcom_reset_controller *reset;
 	struct qcom_cc *cc;
+	struct gdsc_desc *scd;
 	size_t num_clks = desc->num_clks;
 	struct clk_regmap **rclks = desc->clks;
 
@@ -143,15 +144,18 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	devm_add_action(dev, qcom_cc_reset_unregister, &reset->rcdev);
 
 	if (desc->gdscs && desc->num_gdscs) {
-		ret = gdsc_register(dev, desc->gdscs, desc->num_gdscs,
-				    &reset->rcdev, regmap);
+		scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
+		if (!scd)
+			return -ENOMEM;
+		scd->dev = dev;
+		scd->scs = desc->gdscs;
+		scd->num = desc->num_gdscs;
+		ret = gdsc_register(scd, &reset->rcdev, regmap);
 		if (ret)
 			return ret;
+		devm_add_action(dev, qcom_cc_gdsc_unregister, scd);
 	}
 
-	devm_add_action(dev, qcom_cc_gdsc_unregister, dev);
-
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index da9fad8..a164c38 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -201,11 +201,14 @@ static int gdsc_init(struct gdsc *sc)
 	return 0;
 }
 
-int gdsc_register(struct device *dev, struct gdsc **scs, size_t num,
+int gdsc_register(struct gdsc_desc *desc,
 		  struct reset_controller_dev *rcdev, struct regmap *regmap)
 {
 	int i, ret;
 	struct genpd_onecell_data *data;
+	struct device *dev = desc->dev;
+	struct gdsc **scs = desc->scs;
+	size_t num = desc->num;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -228,10 +231,30 @@ int gdsc_register(struct device *dev, struct gdsc **scs, size_t num,
 		data->domains[i] = &scs[i]->pd;
 	}
 
+	/* Add subdomains */
+	for (i = 0; i < num; i++) {
+		if (!scs[i])
+			continue;
+		if (scs[i]->parent)
+			pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+	}
+
 	return of_genpd_add_provider_onecell(dev->of_node, data);
 }
 
-void gdsc_unregister(struct device *dev)
+void gdsc_unregister(struct gdsc_desc *desc)
 {
+	int i;
+	struct device *dev = desc->dev;
+	struct gdsc **scs = desc->scs;
+	size_t num = desc->num;
+
+	/* Remove subdomains */
+	for (i = 0; i < num; i++) {
+		if (!scs[i])
+			continue;
+		if (scs[i]->parent)
+			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+	}
 	of_genpd_del_provider(dev->of_node);
 }
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 5ded268..4e9dfc1 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -41,6 +41,7 @@ struct reset_controller_dev;
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
+	struct generic_pm_domain	*parent;
 	struct regmap			*regmap;
 	unsigned int			gdscr;
 	unsigned int			*cxcs;
@@ -51,18 +52,24 @@ struct gdsc {
 	unsigned int			reset_count;
 };
 
+struct gdsc_desc {
+	struct device *dev;
+	struct gdsc **scs;
+	size_t num;
+};
+
 #ifdef CONFIG_QCOM_GDSC
-int gdsc_register(struct device *, struct gdsc **, size_t n,
-		  struct reset_controller_dev *, struct regmap *);
-void gdsc_unregister(struct device *);
+int gdsc_register(struct gdsc_desc *desc, struct reset_controller_dev *,
+		  struct regmap *);
+void gdsc_unregister(struct gdsc_desc *desc);
 #else
-static inline int gdsc_register(struct device *d, struct gdsc **g, size_t n,
+static inline int gdsc_register(struct gdsc_desc *desc,
 				struct reset_controller_dev *rcdev,
 				struct regmap *r)
 {
 	return -ENOSYS;
 }
 
-static inline void gdsc_unregister(struct device *d) {};
+static inline void gdsc_unregister(struct gdsc_desc *desc) {};
 #endif /* CONFIG_QCOM_GDSC */
 #endif /* __QCOM_GDSC_H__ */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277986 — [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

Some gdsc power domains can have a gds_hw_controller block inside
to help ensure all slave devices within the power domain are idle
before the gdsc is actually switched off.
This is mainly useful in power domains which host a MMU, in which
case its necessary to make sure there are no outstanding MMU operations
or pending bus transactions before the power domain is turned off.

In gdscs with gds_hw_controller block, its necessary to check the
gds_hw_ctrl status bits instead of the ones in gdscr, to determine
the state of the powerdomain.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 38 ++++++++++++++++++++++----------------
 drivers/clk/qcom/gdsc.h |  2 ++
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a164c38..fb2e43c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -42,12 +42,12 @@
 
 #define domain_to_gdsc(domain) container_of(domain, struct gdsc, pd)
 
-static int gdsc_is_enabled(struct gdsc *sc)
+static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
 {
 	u32 val;
 	int ret;
 
-	ret = regmap_read(sc->regmap, sc->gdscr, &val);
+	ret = regmap_read(sc->regmap, reg, &val);
 	if (ret)
 		return ret;
 
@@ -58,30 +58,34 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
 	int ret;
 	u32 val = en ? 0 : SW_COLLAPSE_MASK;
-	u32 check = en ? PWR_ON_MASK : 0;
 	unsigned long timeout;
+	unsigned int status_reg = sc->gdscr;
 
 	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
 	if (ret)
 		return ret;
 
 	timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
-	do {
-		ret = regmap_read(sc->regmap, sc->gdscr, &val);
-		if (ret)
-			return ret;
 
-		if ((val & PWR_ON_MASK) == check)
+	if (sc->gds_hw_ctrl) {
+		status_reg = sc->gds_hw_ctrl;
+	/*
+	 * The gds hw controller asserts/de-asserts the status bit soon
+	 * after it receives a power on/off request from a master.
+	 * The controller then takes around 8 xo cycles to start its internal
+	 * state machine and update the status bit. During this time, the
+	 * status bit does not reflect the true status of the core.
+	 * Add a delay of 1 us between writing to the SW_COLLAPSE bit and
+	 * polling the status bit
+	 */
+		udelay(1);
+	}
+
+	do {
+		if (gdsc_is_enabled(sc, status_reg) == en)
 			return 0;
 	} while (time_before(jiffies, timeout));
 
-	ret = regmap_read(sc->regmap, sc->gdscr, &val);
-	if (ret)
-		return ret;
-
-	if ((val & PWR_ON_MASK) == check)
-		return 0;
-
 	return -ETIMEDOUT;
 }
 
@@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;
 	int on, ret;
+	unsigned int reg;
 
 	/*
 	 * Disable HW trigger: collapse/restore occur based on registers writes.
@@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
 			return ret;
 	}
 
-	on = gdsc_is_enabled(sc);
+	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
+	on = gdsc_is_enabled(sc, reg);
 	if (on < 0)
 		return on;
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 4e9dfc1..66a43be 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -32,6 +32,7 @@ struct reset_controller_dev;
  * @pd: generic power domain
  * @regmap: regmap for MMIO accesses
  * @gdscr: gsdc control register
+ * @gds_hw_ctrl: gds_hw_ctrl register
  * @cxcs: offsets of branch registers to toggle mem/periph bits in
  * @cxc_count: number of @cxcs
  * @pwrsts: Possible powerdomain power states
@@ -44,6 +45,7 @@ struct gdsc {
 	struct generic_pm_domain	*parent;
 	struct regmap			*regmap;
 	unsigned int			gdscr;
+	unsigned int			gds_hw_ctrl;
 	unsigned int			*cxcs;
 	unsigned int			cxc_count;
 	const u8			pwrsts;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280499 — Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

On 11/26, Rajendra Nayak wrote:
> @@ -58,30 +58,34 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>  {
>  	int ret;
>  	u32 val = en ? 0 : SW_COLLAPSE_MASK;
> -	u32 check = en ? PWR_ON_MASK : 0;
>  	unsigned long timeout;
> +	unsigned int status_reg = sc->gdscr;
>  
>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
>  	if (ret)
>  		return ret;
>  
>  	timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
> -	do {
> -		ret = regmap_read(sc->regmap, sc->gdscr, &val);
> -		if (ret)
> -			return ret;
>  
> -		if ((val & PWR_ON_MASK) == check)
> +	if (sc->gds_hw_ctrl) {
> +		status_reg = sc->gds_hw_ctrl;
> +	/*
> +	 * The gds hw controller asserts/de-asserts the status bit soon
> +	 * after it receives a power on/off request from a master.
> +	 * The controller then takes around 8 xo cycles to start its internal
> +	 * state machine and update the status bit. During this time, the
> +	 * status bit does not reflect the true status of the core.
> +	 * Add a delay of 1 us between writing to the SW_COLLAPSE bit and
> +	 * polling the status bit
> +	 */

Please indent this correctly to the udelay indent level.

> +		udelay(1);
> +	}
> +
> +	do {
> +		if (gdsc_is_enabled(sc, status_reg) == en)
>  			return 0;
>  	} while (time_before(jiffies, timeout));
>  
> -	ret = regmap_read(sc->regmap, sc->gdscr, &val);
> -	if (ret)
> -		return ret;
> -
> -	if ((val & PWR_ON_MASK) == check)
> -		return 0;
> -

This opens a bug where we timeout and then the status bit changes
after the timeout. One more check is good and should stay. We
could also change this to ktime instead of jiffies. That would be
a good improvement.

>  	return -ETIMEDOUT;
>  }
>  
> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
>  {
>  	u32 mask, val;
>  	int on, ret;
> +	unsigned int reg;
>  
>  	/*
>  	 * Disable HW trigger: collapse/restore occur based on registers writes.
> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
>  			return ret;
>  	}
>  
> -	on = gdsc_is_enabled(sc);
> +	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> +	on = gdsc_is_enabled(sc, reg);

If the gdsc is voteable, then we need to make sure that the vote
is from us when we boot up. Otherwise the kernel may think that
the gdsc is enabled, but it gets turned off by some other master
later on. I don't know if this causes some sort of problem for
the power domain framework, but we can't rely on the status bit
unless we're sure that we've actually set the register to enable
it. In the normal enable/disable path we'll always know we set
the register, so this really only matters once when we boot up.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280527 — Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

On 12/01/2015 07:52 AM, Stephen Boyd wrote:
> On 11/26, Rajendra Nayak wrote:
>> @@ -58,30 +58,34 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  	int ret;
>>  	u32 val = en ? 0 : SW_COLLAPSE_MASK;
>> -	u32 check = en ? PWR_ON_MASK : 0;
>>  	unsigned long timeout;
>> +	unsigned int status_reg = sc->gdscr;
>>  
>>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
>>  	if (ret)
>>  		return ret;
>>  
>>  	timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
>> -	do {
>> -		ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> -		if (ret)
>> -			return ret;
>>  
>> -		if ((val & PWR_ON_MASK) == check)
>> +	if (sc->gds_hw_ctrl) {
>> +		status_reg = sc->gds_hw_ctrl;
>> +	/*
>> +	 * The gds hw controller asserts/de-asserts the status bit soon
>> +	 * after it receives a power on/off request from a master.
>> +	 * The controller then takes around 8 xo cycles to start its internal
>> +	 * state machine and update the status bit. During this time, the
>> +	 * status bit does not reflect the true status of the core.
>> +	 * Add a delay of 1 us between writing to the SW_COLLAPSE bit and
>> +	 * polling the status bit
>> +	 */
> 
> Please indent this correctly to the udelay indent level.

will do.

> 
>> +		udelay(1);
>> +	}
>> +
>> +	do {
>> +		if (gdsc_is_enabled(sc, status_reg) == en)
>>  			return 0;
>>  	} while (time_before(jiffies, timeout));
>>  
>> -	ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if ((val & PWR_ON_MASK) == check)
>> -		return 0;
>> -
> 
> This opens a bug where we timeout and then the status bit changes
> after the timeout. One more check is good and should stay. We
> could also change this to ktime instead of jiffies. That would be
> a good improvement.

If the status bit does change after timeout maybe the timeout isn't
really enough and needs to be increased?

> 
>>  	return -ETIMEDOUT;
>>  }
>>  
>> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
>>  {
>>  	u32 mask, val;
>>  	int on, ret;
>> +	unsigned int reg;
>>  
>>  	/*
>>  	 * Disable HW trigger: collapse/restore occur based on registers writes.
>> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
>>  			return ret;
>>  	}
>>  
>> -	on = gdsc_is_enabled(sc);
>> +	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
>> +	on = gdsc_is_enabled(sc, reg);
> 
> If the gdsc is voteable, then we need to make sure that the vote
> is from us when we boot up. Otherwise the kernel may think that
> the gdsc is enabled, but it gets turned off by some other master
> later on. I don't know if this causes some sort of problem for
> the power domain framework, but we can't rely on the status bit
> unless we're sure that we've actually set the register to enable
> it. In the normal enable/disable path we'll always know we set
> the register, so this really only matters once when we boot up.

right, thanks for catching this. However if we vote for a votable
GDSC just because its ON at boot (due to someone else having voted)
we won't ever remove the vote keeping it always enabled.

I think a safe way would be to consider all votable gdscs for which
*we* haven't voted explicitly to be disabled at boot?

> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280612 — Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

On 12/01, Rajendra Nayak wrote:
> 
> On 12/01/2015 07:52 AM, Stephen Boyd wrote:
> > On 11/26, Rajendra Nayak wrote:
> > 
> >> +		udelay(1);
> >> +	}
> >> +
> >> +	do {
> >> +		if (gdsc_is_enabled(sc, status_reg) == en)
> >>  			return 0;
> >>  	} while (time_before(jiffies, timeout));
> >>  
> >> -	ret = regmap_read(sc->regmap, sc->gdscr, &val);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	if ((val & PWR_ON_MASK) == check)
> >> -		return 0;
> >> -
> > 
> > This opens a bug where we timeout and then the status bit changes
> > after the timeout. One more check is good and should stay. We
> > could also change this to ktime instead of jiffies. That would be
> > a good improvement.
> 
> If the status bit does change after timeout maybe the timeout isn't
> really enough and needs to be increased?

The problem is more that this isn't a tight loop with interrupts
disabled, so we may schedule the task away for quite some time
before we come back here and test the timeout against the jiffies
value. So it's best to always check the register one more time
after we bail out in case this occurs.

> 
> > 
> >>  	return -ETIMEDOUT;
> >>  }
> >>  
> >> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
> >>  {
> >>  	u32 mask, val;
> >>  	int on, ret;
> >> +	unsigned int reg;
> >>  
> >>  	/*
> >>  	 * Disable HW trigger: collapse/restore occur based on registers writes.
> >> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
> >>  			return ret;
> >>  	}
> >>  
> >> -	on = gdsc_is_enabled(sc);
> >> +	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> >> +	on = gdsc_is_enabled(sc, reg);
> > 
> > If the gdsc is voteable, then we need to make sure that the vote
> > is from us when we boot up. Otherwise the kernel may think that
> > the gdsc is enabled, but it gets turned off by some other master
> > later on. I don't know if this causes some sort of problem for
> > the power domain framework, but we can't rely on the status bit
> > unless we're sure that we've actually set the register to enable
> > it. In the normal enable/disable path we'll always know we set
> > the register, so this really only matters once when we boot up.
> 
> right, thanks for catching this. However if we vote for a votable
> GDSC just because its ON at boot (due to someone else having voted)
> we won't ever remove the vote keeping it always enabled.
> 
> I think a safe way would be to consider all votable gdscs for which
> *we* haven't voted explicitly to be disabled at boot?
> 

Agreed, when we boot we should consider GDSCs that are indicating
they're enabled via the bit 31 status bit but without the sw
enable mask set as "disabled" even though they're actually
enabled by some other master in the SoC.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280991 — Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

[]..

>> >>  	return -ETIMEDOUT;
>> >>  }
>> >>
>> >> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
>> >>  {
>> >>  	u32 mask, val;
>> >>  	int on, ret;
>> >> +	unsigned int reg;
>> >>
>> >>  	/*
>> >>  	 * Disable HW trigger: collapse/restore occur based on registers
>> writes.
>> >> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
>> >>  			return ret;
>> >>  	}
>> >>
>> >> -	on = gdsc_is_enabled(sc);
>> >> +	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
>> >> +	on = gdsc_is_enabled(sc, reg);
>> >
>> > If the gdsc is voteable, then we need to make sure that the vote
>> > is from us when we boot up. Otherwise the kernel may think that
>> > the gdsc is enabled, but it gets turned off by some other master
>> > later on. I don't know if this causes some sort of problem for
>> > the power domain framework, but we can't rely on the status bit
>> > unless we're sure that we've actually set the register to enable
>> > it. In the normal enable/disable path we'll always know we set
>> > the register, so this really only matters once when we boot up.
>>
>> right, thanks for catching this. However if we vote for a votable
>> GDSC just because its ON at boot (due to someone else having voted)
>> we won't ever remove the vote keeping it always enabled.
>>
>> I think a safe way would be to consider all votable gdscs for which
>> *we* haven't voted explicitly to be disabled at boot?
>>
>
> Agreed, when we boot we should consider GDSCs that are indicating
> they're enabled via the bit 31 status bit but without the sw
> enable mask set as "disabled" even though they're actually
> enabled by some other master in the SoC.

Thinking about this a bit more, your earlier suggestion of voting
for the GDSC explicitly seemed to work too, and also seemed cleaner.
genpd ends up removing the vote if there aren't any users as part
of genpd_poweroff_unused()

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277987 — [PATCH 6/6] clk: qcom: mmcc8974: Use gdscs .parent and remove genpd calls

With gdsc driver capable of handling hierarchical power domains,
specify oxili_gdsc as parent of oxilicx_gdsc.

Remove all direct calls to genpd from the mmcc clock driver. The
adding and removing of subdomains is now handled from within
the gdsc driver.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/mmcc-msm8974.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
index 9d790bc..12cea42 100644
--- a/drivers/clk/qcom/mmcc-msm8974.c
+++ b/drivers/clk/qcom/mmcc-msm8974.c
@@ -2400,6 +2400,7 @@ static struct gdsc oxilicx_gdsc = {
 	.pd = {
 		.name = "oxilicx",
 	},
+	.parent = &oxili_gdsc.pd,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -2624,22 +2625,11 @@ static int mmcc_msm8974_probe(struct platform_device *pdev)
 	clk_pll_configure_sr_hpm_lp(&mmpll1, regmap, &mmpll1_config, true);
 	clk_pll_configure_sr_hpm_lp(&mmpll3, regmap, &mmpll3_config, false);
 
-	ret = qcom_cc_really_probe(pdev, &mmcc_msm8974_desc, regmap);
-	if (ret)
-		return ret;
-
-	return pm_genpd_add_subdomain(&oxili_gdsc.pd, &oxilicx_gdsc.pd);
-}
-
-static int mmcc_msm8974_remove(struct platform_device *pdev)
-{
-	pm_genpd_remove_subdomain(&oxili_gdsc.pd, &oxilicx_gdsc.pd);
-	return 0;
+	return qcom_cc_really_probe(pdev, &mmcc_msm8974_desc, regmap);
 }
 
 static struct platform_driver mmcc_msm8974_driver = {
 	.probe		= mmcc_msm8974_probe,
-	.remove		= mmcc_msm8974_remove,
 	.driver		= {
 		.name	= "mmcc-msm8974",
 		.of_match_table = mmcc_msm8974_match_table,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web