[PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

Recent Qualcomm platforms have APCS collapse-vote registers that allow
for sharing GDSCs with other masters (e.g. LPASS).
    
Add support for using such vote registers instead of the control
register when updating the GDSC power state.

Note that the gcc-sc8280xp driver has not yet been merged. [1]

Johan


[1] https://lore.kernel.org/all/20220505025457.1693716-1-bjorn.andersson@linaro.org/


Johan Hovold (3):
  clk: qcom: gdsc: add collapse-bit helper
  clk: qcom: gdsc: add support for collapse-vote registers
  clk: qcom: gcc-sc8280xp: use collapse-voting for PCIe GDSCs

 drivers/clk/qcom/gcc-sc8280xp.c | 21 +++++++++++++++++++++
 drivers/clk/qcom/gdsc.c         | 28 ++++++++++++++++++++++------
 drivers/clk/qcom/gdsc.h         |  4 ++++
 3 files changed, 47 insertions(+), 6 deletions(-)

-- 
2.35.1

[PATCH 1/3] clk: qcom: gdsc: add collapse-bit helper

[Johan Hovold]

Add a helper for updating the SW_COLLAPSE bit during initialisation and
state updates.

Note that the update during initialisation was relying on the
SW_COLLAPSE bit not having been set earlier rather than passing in zero
explicitly to clear the collapse vote.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 44520efc6c72..c676416e685f 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -132,10 +132,24 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
 	return -ETIMEDOUT;
 }
 
+static int gdsc_update_collapse_bit(struct gdsc *sc, bool val)
+{
+	u32 reg, mask;
+	int ret;
+
+	reg = sc->gdscr;
+	mask = SW_COLLAPSE_MASK;
+
+	ret = regmap_update_bits(sc->regmap, reg, mask, val ? mask : 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 {
 	int ret;
-	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
 
 	if (status == GDSC_ON && sc->rsupply) {
 		ret = regulator_enable(sc->rsupply);
@@ -143,9 +157,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 			return ret;
 	}
 
-	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
-	if (ret)
-		return ret;
+	ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
 
 	/* If disabling votable gdscs, don't poll on status */
 	if ((sc->flags & VOTABLE) && status == GDSC_OFF) {
@@ -425,8 +437,7 @@ static int gdsc_init(struct gdsc *sc)
 		 * If a Votable GDSC is ON, make sure we have a Vote.
 		 */
 		if (sc->flags & VOTABLE) {
-			ret = regmap_update_bits(sc->regmap, sc->gdscr,
-						 SW_COLLAPSE_MASK, val);
+			ret = gdsc_update_collapse_bit(sc, false);
 			if (ret)
 				return ret;
 		}
-- 
2.35.1

Re: [PATCH 1/3] clk: qcom: gdsc: add collapse-bit helper

[Dmitry Baryshkov]

On Fri, 20 May 2022 at 13:10, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Add a helper for updating the SW_COLLAPSE bit during initialisation and
> state updates.
>


> Note that the update during initialisation was relying on the
> SW_COLLAPSE bit not having been set earlier rather than passing in zero
> explicitly to clear the collapse vote.

I think this part deserves a separate commit with proper Fixes: tag.

>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 44520efc6c72..c676416e685f 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -132,10 +132,24 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>         return -ETIMEDOUT;
>  }
>
> +static int gdsc_update_collapse_bit(struct gdsc *sc, bool val)
> +{
> +       u32 reg, mask;
> +       int ret;
> +
> +       reg = sc->gdscr;
> +       mask = SW_COLLAPSE_MASK;
> +
> +       ret = regmap_update_bits(sc->regmap, reg, mask, val ? mask : 0);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
>  static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>  {
>         int ret;
> -       u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
>
>         if (status == GDSC_ON && sc->rsupply) {
>                 ret = regulator_enable(sc->rsupply);
> @@ -143,9 +157,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>                         return ret;
>         }
>
> -       ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
> -       if (ret)
> -               return ret;
> +       ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
>
>         /* If disabling votable gdscs, don't poll on status */
>         if ((sc->flags & VOTABLE) && status == GDSC_OFF) {
> @@ -425,8 +437,7 @@ static int gdsc_init(struct gdsc *sc)
>                  * If a Votable GDSC is ON, make sure we have a Vote.
>                  */
>                 if (sc->flags & VOTABLE) {
> -                       ret = regmap_update_bits(sc->regmap, sc->gdscr,
> -                                                SW_COLLAPSE_MASK, val);
> +                       ret = gdsc_update_collapse_bit(sc, false);
>                         if (ret)
>                                 return ret;
>                 }
> --
> 2.35.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] clk: qcom: gdsc: add collapse-bit helper

[Johan Hovold]

On Fri, May 20, 2022 at 02:50:17PM +0300, Dmitry Baryshkov wrote:
> On Fri, 20 May 2022 at 13:10, Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > Add a helper for updating the SW_COLLAPSE bit during initialisation and
> > state updates.
> >
> 
> 
> > Note that the update during initialisation was relying on the
> > SW_COLLAPSE bit not having been set earlier rather than passing in zero
> > explicitly to clear the collapse vote.
> 
> I think this part deserves a separate commit with proper Fixes: tag.

No, it's not a bug. The value passed in is explicitly set a bit higher
up in the same function so that the SW_COLLAPSE bit is (currently) never
set.

It mostly just looks weird and probably wasn't intentional.

> > @@ -425,8 +437,7 @@ static int gdsc_init(struct gdsc *sc)
> >                  * If a Votable GDSC is ON, make sure we have a Vote.
> >                  */
> >                 if (sc->flags & VOTABLE) {
> > -                       ret = regmap_update_bits(sc->regmap, sc->gdscr,
> > -                                                SW_COLLAPSE_MASK, val);
> > +                       ret = gdsc_update_collapse_bit(sc, false);
> >                         if (ret)
> >                                 return ret;
> >                 }

Johan

[PATCH 2/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

Recent Qualcomm platforms have APCS collapse-vote registers that allow
for sharing GDSCs with other masters (e.g. LPASS).

Add support for using such vote registers instead of the control
register when updating the GDSC power state.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/clk/qcom/gdsc.c | 9 +++++++--
 drivers/clk/qcom/gdsc.h | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index c676416e685f..6f746158d28f 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -137,8 +137,13 @@ static int gdsc_update_collapse_bit(struct gdsc *sc, bool val)
 	u32 reg, mask;
 	int ret;
 
-	reg = sc->gdscr;
-	mask = SW_COLLAPSE_MASK;
+	if (sc->collapse_mask) {
+		reg = sc->collapse_ctrl;
+		mask = sc->collapse_mask;
+	} else {
+		reg = sc->gdscr;
+		mask = SW_COLLAPSE_MASK;
+	}
 
 	ret = regmap_update_bits(sc->regmap, reg, mask, val ? mask : 0);
 	if (ret)
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index ad313d7210bd..5de48c9439b2 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -18,6 +18,8 @@ struct reset_controller_dev;
  * @pd: generic power domain
  * @regmap: regmap for MMIO accesses
  * @gdscr: gsdc control register
+ * @collapse_ctrl: APCS collapse-vote register
+ * @collapse_mask: APCS collapse-vote mask
  * @gds_hw_ctrl: gds_hw_ctrl register
  * @cxcs: offsets of branch registers to toggle mem/periph bits in
  * @cxc_count: number of @cxcs
@@ -35,6 +37,8 @@ struct gdsc {
 	struct generic_pm_domain	*parent;
 	struct regmap			*regmap;
 	unsigned int			gdscr;
+	unsigned int			collapse_ctrl;
+	unsigned int			collapse_mask;
 	unsigned int			gds_hw_ctrl;
 	unsigned int			clamp_io_ctrl;
 	unsigned int			*cxcs;
-- 
2.35.1

Re: [PATCH 2/3] clk: qcom: gdsc: add support for collapse-vote registers

[Stephen Boyd]

Quoting Johan Hovold (2022-05-20 03:09:47)
> Recent Qualcomm platforms have APCS collapse-vote registers that allow
> for sharing GDSCs with other masters (e.g. LPASS).

Maybe just say 'with other subsystems' because LPASS is an entire
subsystem.

> 
> Add support for using such vote registers instead of the control
> register when updating the GDSC power state.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Re: [PATCH 2/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

On Fri, May 20, 2022 at 08:52:26PM -0700, Stephen Boyd wrote:
> Quoting Johan Hovold (2022-05-20 03:09:47)
> > Recent Qualcomm platforms have APCS collapse-vote registers that allow
> > for sharing GDSCs with other masters (e.g. LPASS).
> 
> Maybe just say 'with other subsystems' because LPASS is an entire
> subsystem.

The term "subsystem" is too broad and also has a different meaning in
Linux.

The vendor kernel uses "masters" here which is clear enough and
presumably matches their documentation.

Johan

Re: [PATCH 2/3] clk: qcom: gdsc: add support for collapse-vote registers

[Stephen Boyd]

Quoting Johan Hovold (2022-05-23 05:04:44)
> On Fri, May 20, 2022 at 08:52:26PM -0700, Stephen Boyd wrote:
> > Quoting Johan Hovold (2022-05-20 03:09:47)
> > > Recent Qualcomm platforms have APCS collapse-vote registers that allow
> > > for sharing GDSCs with other masters (e.g. LPASS).
> > 
> > Maybe just say 'with other subsystems' because LPASS is an entire
> > subsystem.
> 
> The term "subsystem" is too broad and also has a different meaning in
> Linux.
> 
> The vendor kernel uses "masters" here which is clear enough and
> presumably matches their documentation.
> 

How about "voter"? Then it isn't confused with linux subsystems.

Re: [PATCH 2/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

On Thu, May 26, 2022 at 11:32:17AM -0700, Stephen Boyd wrote:
> Quoting Johan Hovold (2022-05-23 05:04:44)
> > On Fri, May 20, 2022 at 08:52:26PM -0700, Stephen Boyd wrote:
> > > Quoting Johan Hovold (2022-05-20 03:09:47)
> > > > Recent Qualcomm platforms have APCS collapse-vote registers that allow
> > > > for sharing GDSCs with other masters (e.g. LPASS).
> > > 
> > > Maybe just say 'with other subsystems' because LPASS is an entire
> > > subsystem.
> > 
> > The term "subsystem" is too broad and also has a different meaning in
> > Linux.
> > 
> > The vendor kernel uses "masters" here which is clear enough and
> > presumably matches their documentation.
> > 
> 
> How about "voter"? Then it isn't confused with linux subsystems.

Yeah, voters vote, but that's not very informative, is it?

I don't see any reason to obfuscate the commit message here.

Johan

[PATCH 3/3] clk: qcom: gcc-sc8280xp: use collapse-voting for PCIe GDSCs

[Johan Hovold]

The PCIe GDSCs can be shared with other masters and should use the APCS
collapse-vote register when updating the power state.

This is specifically also needed to be able to disable power domains
that have been enabled by boot firmware using the vote register.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/clk/qcom/gcc-sc8280xp.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
index 887db5324ab8..4d7db13ed708 100644
--- a/drivers/clk/qcom/gcc-sc8280xp.c
+++ b/drivers/clk/qcom/gcc-sc8280xp.c
@@ -6778,58 +6778,79 @@ static struct clk_branch gcc_video_vcodec_throttle_clk = {
 
 static struct gdsc pcie_0_tunnel_gdsc = {
 	.gdscr = 0xa4004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(0),
 	.pd = {
 		.name = "pcie_0_tunnel_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_1_tunnel_gdsc = {
 	.gdscr = 0x8d004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(1),
 	.pd = {
 		.name = "pcie_1_tunnel_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_2a_gdsc = {
 	.gdscr = 0x9d004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(2),
 	.pd = {
 		.name = "pcie_2a_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_2b_gdsc = {
 	.gdscr = 0x9e004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(3),
 	.pd = {
 		.name = "pcie_2b_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_3a_gdsc = {
 	.gdscr = 0xa0004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(4),
 	.pd = {
 		.name = "pcie_3a_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_3b_gdsc = {
 	.gdscr = 0xa2004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(5),
 	.pd = {
 		.name = "pcie_3b_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_4_gdsc = {
 	.gdscr = 0x6b004,
+	.collapse_ctrl = 0x52128,
+	.collapse_mask = BIT(6),
 	.pd = {
 		.name = "pcie_4_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc ufs_card_gdsc = {
-- 
2.35.1

Re: [PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Stephen Boyd]

Please add Qualcomm on code for their hardware :)

I did a translation from codeaurora but I don't know if Rajendra's will
work.

Quoting Johan Hovold (2022-05-20 03:09:45)
> Recent Qualcomm platforms have APCS collapse-vote registers that allow
> for sharing GDSCs with other masters (e.g. LPASS).

How is it different from the voting logic that already exists in the
gdsc file? Now every subsystem has to vote for off in addition to voting
for on?

>     
> Add support for using such vote registers instead of the control
> register when updating the GDSC power state.

Re: [PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

On Fri, May 20, 2022 at 08:51:09PM -0700, Stephen Boyd wrote:
> Please add Qualcomm on code for their hardware :)
> 
> I did a translation from codeaurora but I don't know if Rajendra's will
> work.

These addresses need to be added to .mailmap.

> Quoting Johan Hovold (2022-05-20 03:09:45)
> > Recent Qualcomm platforms have APCS collapse-vote registers that allow
> > for sharing GDSCs with other masters (e.g. LPASS).
> 
> How is it different from the voting logic that already exists in the
> gdsc file? Now every subsystem has to vote for off in addition to voting
> for on?

No, the voting logic is unchanged (i.e. enabling by clearing a collapse
bit).

The difference is just that a separate register register is used for the
voting.

> >     
> > Add support for using such vote registers instead of the control
> > register when updating the GDSC power state.

Johan

Re: [PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Stephen Boyd]

Quoting Johan Hovold (2022-05-23 02:32:50)
> On Fri, May 20, 2022 at 08:51:09PM -0700, Stephen Boyd wrote:
> > Please add Qualcomm on code for their hardware :)
> > 
> > I did a translation from codeaurora but I don't know if Rajendra's will
> > work.
> 
> These addresses need to be added to .mailmap.

Patches welcome :)

> 
> > Quoting Johan Hovold (2022-05-20 03:09:45)
> > > Recent Qualcomm platforms have APCS collapse-vote registers that allow
> > > for sharing GDSCs with other masters (e.g. LPASS).
> > 
> > How is it different from the voting logic that already exists in the
> > gdsc file? Now every subsystem has to vote for off in addition to voting
> > for on?
> 
> No, the voting logic is unchanged (i.e. enabling by clearing a collapse
> bit).
> 
> The difference is just that a separate register register is used for the
> voting.
> 

Ok. Got it.

Re: [PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Johan Hovold]

On Fri, May 20, 2022 at 12:09:45PM +0200, Johan Hovold wrote:
> Recent Qualcomm platforms have APCS collapse-vote registers that allow
> for sharing GDSCs with other masters (e.g. LPASS).
>     
> Add support for using such vote registers instead of the control
> register when updating the GDSC power state.
> 
> Note that the gcc-sc8280xp driver has not yet been merged. [1]

The sc8280xp driver has been merged so this series could go in now.

Bjorn?
 
> [1] https://lore.kernel.org/all/20220505025457.1693716-1-bjorn.andersson@linaro.org/
> 
> 
> Johan Hovold (3):
>   clk: qcom: gdsc: add collapse-bit helper
>   clk: qcom: gdsc: add support for collapse-vote registers
>   clk: qcom: gcc-sc8280xp: use collapse-voting for PCIe GDSCs
> 
>  drivers/clk/qcom/gcc-sc8280xp.c | 21 +++++++++++++++++++++
>  drivers/clk/qcom/gdsc.c         | 28 ++++++++++++++++++++++------
>  drivers/clk/qcom/gdsc.h         |  4 ++++
>  3 files changed, 47 insertions(+), 6 deletions(-)

Johan

Re: [PATCH 0/3] clk: qcom: gdsc: add support for collapse-vote registers

[Bjorn Andersson]

On Fri, 20 May 2022 12:09:45 +0200, Johan Hovold wrote:
> Recent Qualcomm platforms have APCS collapse-vote registers that allow
> for sharing GDSCs with other masters (e.g. LPASS).
> 
> Add support for using such vote registers instead of the control
> register when updating the GDSC power state.
> 
> Note that the gcc-sc8280xp driver has not yet been merged. [1]
> 
> [...]

Applied, thanks!

[1/3] clk: qcom: gdsc: add collapse-bit helper
      commit: e73cb8527c597598599119fcd9c7d1752d9e9fd7
[2/3] clk: qcom: gdsc: add support for collapse-vote registers
      commit: 77ea2bd72da4f61f59ad2e839babe83849f35dea
[3/3] clk: qcom: gcc-sc8280xp: use collapse-voting for PCIe GDSCs
      commit: 8d114b94fc39210b88b203b57aaf04836a87a4f0

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>