[PATCH 0/2] mfd: twl: Add clock for TWL6030

[PATCH 0/2] mfd: twl: Add clock for TWL6030

[Andreas Kemnade]

Previously the clock support for only implemented for TWL6032 so add
it also for the TWL6030. There are devices out there where especially
WLAN only works if these clocks are enabled by some patched U-Boot.
This allows to explicitely specify the clock requirements.

Andreas Kemnade (2):
  mfd: twl-core: Add a clock subdevice for the TWL6030
  clk: twl: add TWL6030 support

 drivers/clk/clk-twl.c  | 97 +++++++++++++++++++++++++++++++++++++++++-
 drivers/mfd/twl-core.c | 32 +++++++++-----
 2 files changed, 117 insertions(+), 12 deletions(-)

-- 
2.39.5

[PATCH 1/2] mfd: twl-core: Add a clock subdevice for the TWL6030

[Andreas Kemnade]

Also the TWL6030 has some clocks, so add a subdevice for that.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/twl-core.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index c130ffef182f..c981922f62d5 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -711,6 +711,10 @@ static struct of_dev_auxdata twl_auxdata_lookup[] = {
 	{ /* sentinel */ },
 };
 
+static const struct mfd_cell twl6030_cells[] = {
+	{ .name = "twl6030-clk" },
+};
+
 static const struct mfd_cell twl6032_cells[] = {
 	{ .name = "twl6032-clk" },
 };
@@ -861,17 +865,25 @@ twl_probe(struct i2c_client *client)
 				 TWL4030_DCDC_GLOBAL_CFG);
 	}
 
-	if (id->driver_data == (TWL6030_CLASS | TWL6032_SUBCLASS)) {
-		status = devm_mfd_add_devices(&client->dev,
-					      PLATFORM_DEVID_NONE,
-					      twl6032_cells,
-					      ARRAY_SIZE(twl6032_cells),
-					      NULL, 0, NULL);
-		if (status < 0)
-			goto free;
-	}
-
 	if (twl_class_is_6030()) {
+		if (id->driver_data & TWL6032_SUBCLASS) {
+			status = devm_mfd_add_devices(&client->dev,
+						      PLATFORM_DEVID_NONE,
+						      twl6032_cells,
+						      ARRAY_SIZE(twl6032_cells),
+						      NULL, 0, NULL);
+			if (status < 0)
+				goto free;
+		} else {
+			status = devm_mfd_add_devices(&client->dev,
+						      PLATFORM_DEVID_NONE,
+						      twl6030_cells,
+						      ARRAY_SIZE(twl6030_cells),
+						      NULL, 0, NULL);
+			if (status < 0)
+				goto free;
+		}
+
 		if (of_device_is_system_power_controller(node)) {
 			if (!pm_power_off)
 				pm_power_off = twl6030_power_off;
-- 
2.39.5

Re: [PATCH 1/2] mfd: twl-core: Add a clock subdevice for the TWL6030

[Roger Quadros]

On 24/09/2024 13:36, Andreas Kemnade wrote:
> Also the TWL6030 has some clocks, so add a subdevice for that.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH 1/2] mfd: twl-core: Add a clock subdevice for the TWL6030

[Lee Jones]

On Tue, 24 Sep 2024, Andreas Kemnade wrote:

> Also the TWL6030 has some clocks, so add a subdevice for that.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/mfd/twl-core.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index c130ffef182f..c981922f62d5 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -711,6 +711,10 @@ static struct of_dev_auxdata twl_auxdata_lookup[] = {
>  	{ /* sentinel */ },
>  };
>  
> +static const struct mfd_cell twl6030_cells[] = {
> +	{ .name = "twl6030-clk" },
> +};
> +
>  static const struct mfd_cell twl6032_cells[] = {
>  	{ .name = "twl6032-clk" },
>  };
> @@ -861,17 +865,25 @@ twl_probe(struct i2c_client *client)
>  				 TWL4030_DCDC_GLOBAL_CFG);
>  	}
>  
> -	if (id->driver_data == (TWL6030_CLASS | TWL6032_SUBCLASS)) {
> -		status = devm_mfd_add_devices(&client->dev,
> -					      PLATFORM_DEVID_NONE,
> -					      twl6032_cells,
> -					      ARRAY_SIZE(twl6032_cells),
> -					      NULL, 0, NULL);
> -		if (status < 0)
> -			goto free;
> -	}
> -
>  	if (twl_class_is_6030()) {
> +		if (id->driver_data & TWL6032_SUBCLASS) {
> +			status = devm_mfd_add_devices(&client->dev,
> +						      PLATFORM_DEVID_NONE,
> +						      twl6032_cells,
> +						      ARRAY_SIZE(twl6032_cells),
> +						      NULL, 0, NULL);
> +			if (status < 0)
> +				goto free;
> +		} else {
> +			status = devm_mfd_add_devices(&client->dev,
> +						      PLATFORM_DEVID_NONE,
> +						      twl6030_cells,
> +						      ARRAY_SIZE(twl6030_cells),
> +						      NULL, 0, NULL);
> +			if (status < 0)
> +				goto free;
> +		}
> +

Before this gets too crazy, how about:

> +		if (id->driver_data & TWL6032_SUBCLASS) {
> +			cells = twl6032_cells;
> +			num_cells = ARRAY_SIZE(twl6032_cells);
> +		} else {
> +			cells = twl6030_cells;
> +			num_cells = ARRAY_SIZE(twl6030_cells);
> +		}
> +
> +		status = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> +					      cells, num_cells, NULL, 0, NULL);
> +		if (status < 0)
> +			goto free;


>  		if (of_device_is_system_power_controller(node)) {
>  			if (!pm_power_off)
>  				pm_power_off = twl6030_power_off;
> -- 
> 2.39.5
> 

-- 
Lee Jones [李琼斯]

[PATCH 2/2] clk: twl: add TWL6030 support

[Andreas Kemnade]

The TWL6030 has similar clocks, so add support for it. Take care of the
resource grouping handling needed.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/clk/clk-twl.c | 97 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
index eab9d3c8ed8a..194f11ac5e14 100644
--- a/drivers/clk/clk-twl.c
+++ b/drivers/clk/clk-twl.c
@@ -11,10 +11,22 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
-#define VREG_STATE              2
+#define VREG_STATE		2
+#define VREG_GRP		0
 #define TWL6030_CFG_STATE_OFF   0x00
 #define TWL6030_CFG_STATE_ON    0x01
 #define TWL6030_CFG_STATE_MASK  0x03
+#define TWL6030_CFG_STATE_GRP_SHIFT	5
+#define TWL6030_CFG_STATE_APP_SHIFT	2
+#define TWL6030_CFG_STATE_MASK		0x03
+#define TWL6030_CFG_STATE_APP_MASK	(0x03 << TWL6030_CFG_STATE_APP_SHIFT)
+#define TWL6030_CFG_STATE_APP(v)	(((v) & TWL6030_CFG_STATE_APP_MASK) >>\
+						TWL6030_CFG_STATE_APP_SHIFT)
+#define P1_GRP BIT(0) /* processor power group */
+#define ALL_GRP (BIT(0) | BIT(1) | BIT(2))
+
+#define DRIVER_DATA_TWL6030 0
+#define DRIVER_DATA_TWL6032 1
 
 struct twl_clock_info {
 	struct device *dev;
@@ -53,6 +65,49 @@ static unsigned long twl_clks_recalc_rate(struct clk_hw *hw,
 	return 32768;
 }
 
+static int twl6030_clks_prepare(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+	int grp;
+
+	grp = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+	if (grp < 0)
+		return grp;
+
+	return twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+			    grp << TWL6030_CFG_STATE_GRP_SHIFT |
+			    TWL6030_CFG_STATE_ON);
+}
+
+static void twl6030_clks_unprepare(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+
+	twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+		     ALL_GRP << TWL6030_CFG_STATE_GRP_SHIFT |
+		     TWL6030_CFG_STATE_OFF);
+}
+
+static int twl6030_clks_is_prepared(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+	int val;
+
+	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+	if (val < 0)
+		return val;
+
+	if (!(val & P1_GRP))
+		return 0;
+
+	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE);
+	if (val < 0)
+		return val;
+
+	val = TWL6030_CFG_STATE_APP(val);
+	return val == TWL6030_CFG_STATE_ON;
+}
+
 static int twl6032_clks_prepare(struct clk_hw *hw)
 {
 	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
@@ -93,6 +148,13 @@ static int twl6032_clks_is_prepared(struct clk_hw *hw)
 	return val == TWL6030_CFG_STATE_ON;
 }
 
+static const struct clk_ops twl6030_clks_ops = {
+	.prepare	= twl6030_clks_prepare,
+	.unprepare	= twl6030_clks_unprepare,
+	.is_prepared	= twl6030_clks_is_prepared,
+	.recalc_rate	= twl_clks_recalc_rate,
+};
+
 static const struct clk_ops twl6032_clks_ops = {
 	.prepare	= twl6032_clks_prepare,
 	.unprepare	= twl6032_clks_unprepare,
@@ -105,6 +167,28 @@ struct twl_clks_data {
 	u8 base;
 };
 
+static const struct twl_clks_data twl6030_clks[] = {
+	{
+		.init = {
+			.name = "clk32kg",
+			.ops = &twl6030_clks_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+		.base = 0x8C,
+	},
+	{
+		.init = {
+			.name = "clk32kaudio",
+			.ops = &twl6030_clks_ops,
+			.flags = CLK_IGNORE_UNUSED,
+		},
+		.base = 0x8F,
+	},
+	{
+		/* sentinel */
+	}
+};
+
 static const struct twl_clks_data twl6032_clks[] = {
 	{
 		.init = {
@@ -127,6 +211,11 @@ static const struct twl_clks_data twl6032_clks[] = {
 	}
 };
 
+static const struct twl_clks_data *const twl_clks[] = {
+	[DRIVER_DATA_TWL6030] = twl6030_clks,
+	[DRIVER_DATA_TWL6032] = twl6032_clks,
+};
+
 static int twl_clks_probe(struct platform_device *pdev)
 {
 	struct clk_hw_onecell_data *clk_data;
@@ -137,7 +226,7 @@ static int twl_clks_probe(struct platform_device *pdev)
 	int i;
 	int count;
 
-	hw_data = twl6032_clks;
+	hw_data = twl_clks[platform_get_device_id(pdev)->driver_data];
 	for (count = 0; hw_data[count].init.name; count++)
 		;
 
@@ -176,7 +265,11 @@ static int twl_clks_probe(struct platform_device *pdev)
 
 static const struct platform_device_id twl_clks_id[] = {
 	{
+		.name = "twl6030-clk",
+		.driver_data = DRIVER_DATA_TWL6030,
+	}, {
 		.name = "twl6032-clk",
+		.driver_data = DRIVER_DATA_TWL6032,
 	}, {
 		/* sentinel */
 	}
-- 
2.39.5

Re: [PATCH 2/2] clk: twl: add TWL6030 support

[Roger Quadros]

Hi Andreas,

On 24/09/2024 13:36, Andreas Kemnade wrote:
> The TWL6030 has similar clocks, so add support for it. Take care of the
> resource grouping handling needed.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/clk/clk-twl.c | 97 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c

You will have to add information about TWL6030 to Kconfig.

"config CLK_TWL
        tristate "Clock driver for the TWL PMIC family"
        depends on TWL4030_CORE
        help
          Enable support for controlling the clock resources on TWL family
          PMICs. These devices have some 32K clock outputs which can be
          controlled by software. For now, only the TWL6032 clocks are
          supported."

> index eab9d3c8ed8a..194f11ac5e14 100644
> --- a/drivers/clk/clk-twl.c
> +++ b/drivers/clk/clk-twl.c
> @@ -11,10 +11,22 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> -#define VREG_STATE              2
> +#define VREG_STATE		2
> +#define VREG_GRP		0
>  #define TWL6030_CFG_STATE_OFF   0x00
>  #define TWL6030_CFG_STATE_ON    0x01
>  #define TWL6030_CFG_STATE_MASK  0x03
> +#define TWL6030_CFG_STATE_GRP_SHIFT	5
> +#define TWL6030_CFG_STATE_APP_SHIFT	2
> +#define TWL6030_CFG_STATE_MASK		0x03

unnecessary change?
let's leave TWL6030_CFG_STATE_MASK before TWL6030_CFG_STATE_GRP_SHIFT.

> +#define TWL6030_CFG_STATE_APP_MASK	(0x03 << TWL6030_CFG_STATE_APP_SHIFT)
> +#define TWL6030_CFG_STATE_APP(v)	(((v) & TWL6030_CFG_STATE_APP_MASK) >>\
> +						TWL6030_CFG_STATE_APP_SHIFT)
> +#define P1_GRP BIT(0) /* processor power group */
What are the other power groups? Looks like there are 2 more from below code.

> +#define ALL_GRP (BIT(0) | BIT(1) | BIT(2))
Please use earlier defined groups (P1_GRP, etc) instead of re-defining with BIT().

> +
> +#define DRIVER_DATA_TWL6030 0
> +#define DRIVER_DATA_TWL6032 1
>  
>  struct twl_clock_info {
>  	struct device *dev;
> @@ -53,6 +65,49 @@ static unsigned long twl_clks_recalc_rate(struct clk_hw *hw,
>  	return 32768;
>  }
>  
> +static int twl6030_clks_prepare(struct clk_hw *hw)
> +{
> +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +	int grp;
> +
> +	grp = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> +	if (grp < 0)
> +		return grp;
> +
> +	return twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> +			    grp << TWL6030_CFG_STATE_GRP_SHIFT |
> +			    TWL6030_CFG_STATE_ON);
> +}
> +
> +static void twl6030_clks_unprepare(struct clk_hw *hw)
> +{
> +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +
> +	twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> +		     ALL_GRP << TWL6030_CFG_STATE_GRP_SHIFT |

Why are you unpreparing ALL_GRP? In prepare you only used VREG_GRP.

> +		     TWL6030_CFG_STATE_OFF);
> +}
> +
> +static int twl6030_clks_is_prepared(struct clk_hw *hw)
> +{
> +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +	int val;
> +
> +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> +	if (val < 0)
> +		return val;
> +
> +	if (!(val & P1_GRP))
> +		return 0;
> +
> +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE);
> +	if (val < 0)
> +		return val;
> +
> +	val = TWL6030_CFG_STATE_APP(val);
> +	return val == TWL6030_CFG_STATE_ON

Is there a possibility that after calling twl6030_clks_prepare()
the clock can still remain OFF?
If not then we could just use a private flag to indicate clock
prepared status and return that instead of reading the registers again.


> +}
> +
>  static int twl6032_clks_prepare(struct clk_hw *hw)
>  {
>  	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> @@ -93,6 +148,13 @@ static int twl6032_clks_is_prepared(struct clk_hw *hw)
>  	return val == TWL6030_CFG_STATE_ON;
>  }
>  
> +static const struct clk_ops twl6030_clks_ops = {
> +	.prepare	= twl6030_clks_prepare,
> +	.unprepare	= twl6030_clks_unprepare,
> +	.is_prepared	= twl6030_clks_is_prepared,
> +	.recalc_rate	= twl_clks_recalc_rate,
> +};

Instead of re-defining all the clock ops can't we just reuse the
existing twl6032 clock ops?

We just need to tackle the twl6030 specific stuff inside the ops based on
some platform driver data flag.

> +
>  static const struct clk_ops twl6032_clks_ops = {
>  	.prepare	= twl6032_clks_prepare,
>  	.unprepare	= twl6032_clks_unprepare,
> @@ -105,6 +167,28 @@ struct twl_clks_data {
>  	u8 base;
>  };
>  
> +static const struct twl_clks_data twl6030_clks[] = {
> +	{
> +		.init = {
> +			.name = "clk32kg",
> +			.ops = &twl6030_clks_ops,
> +			.flags = CLK_IGNORE_UNUSED,
> +		},
> +		.base = 0x8C,
> +	},
> +	{
> +		.init = {
> +			.name = "clk32kaudio",
> +			.ops = &twl6030_clks_ops,
> +			.flags = CLK_IGNORE_UNUSED,
> +		},
> +		.base = 0x8F,
> +	},
> +	{
> +		/* sentinel */
> +	}
> +};
> +

This clock data is identical to twl6032.
We could implement the same feature with a lot less code if we just
reuse the data and clock ops.

>  static const struct twl_clks_data twl6032_clks[] = {
>  	{
>  		.init = {
> @@ -127,6 +211,11 @@ static const struct twl_clks_data twl6032_clks[] = {
>  	}
>  };
>  
> +static const struct twl_clks_data *const twl_clks[] = {
> +	[DRIVER_DATA_TWL6030] = twl6030_clks,
> +	[DRIVER_DATA_TWL6032] = twl6032_clks,
> +};
> +
>  static int twl_clks_probe(struct platform_device *pdev)
>  {
>  	struct clk_hw_onecell_data *clk_data;
> @@ -137,7 +226,7 @@ static int twl_clks_probe(struct platform_device *pdev)
>  	int i;
>  	int count;
>  
> -	hw_data = twl6032_clks;
> +	hw_data = twl_clks[platform_get_device_id(pdev)->driver_data];
>  	for (count = 0; hw_data[count].init.name; count++)
>  		;
>  
> @@ -176,7 +265,11 @@ static int twl_clks_probe(struct platform_device *pdev)
>  
>  static const struct platform_device_id twl_clks_id[] = {
>  	{
> +		.name = "twl6030-clk",
> +		.driver_data = DRIVER_DATA_TWL6030,
> +	}, {
>  		.name = "twl6032-clk",
> +		.driver_data = DRIVER_DATA_TWL6032,
>  	}, {
>  		/* sentinel */
>  	}

-- 
cheers,
-roger

Re: [PATCH 2/2] clk: twl: add TWL6030 support

[Andreas Kemnade]

Am Wed, 25 Sep 2024 10:07:29 +0300
schrieb Roger Quadros <rogerq@kernel.org>:

[...]
> > +static void twl6030_clks_unprepare(struct clk_hw *hw)
> > +{
> > +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > +
> > +	twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> > +		     ALL_GRP << TWL6030_CFG_STATE_GRP_SHIFT |  
> 
> Why are you unpreparing ALL_GRP? In prepare you only used VREG_GRP.
>
well, if we want control, then I think using every group to turn it off
into a defined state is a good idea.


> > +		     TWL6030_CFG_STATE_OFF);
> > +}
> > +
> > +static int twl6030_clks_is_prepared(struct clk_hw *hw)
> > +{
> > +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > +	int val;
> > +
> > +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (!(val & P1_GRP))
> > +		return 0;
> > +
> > +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER,
> > VREG_STATE);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	val = TWL6030_CFG_STATE_APP(val);
> > +	return val == TWL6030_CFG_STATE_ON  
> 
> Is there a possibility that after calling twl6030_clks_prepare()
> the clock can still remain OFF?

I do not see a reason. 

> If not then we could just use a private flag to indicate clock
> prepared status and return that instead of reading the registers
> again.
>
The clock core already uses prepare_count if no is_prepared() is
defined.
So this prepare functions can just be dropped.

> 
> > +}
> > +
> >  static int twl6032_clks_prepare(struct clk_hw *hw)
> >  {
> >  	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > @@ -93,6 +148,13 @@ static int twl6032_clks_is_prepared(struct
> > clk_hw *hw) return val == TWL6030_CFG_STATE_ON;
> >  }
> >  
> > +static const struct clk_ops twl6030_clks_ops = {
> > +	.prepare	= twl6030_clks_prepare,
> > +	.unprepare	= twl6030_clks_unprepare,
> > +	.is_prepared	= twl6030_clks_is_prepared,
> > +	.recalc_rate	= twl_clks_recalc_rate,
> > +};  
> 
> Instead of re-defining all the clock ops can't we just reuse the
> existing twl6032 clock ops?
> 
> We just need to tackle the twl6030 specific stuff inside the ops
> based on some platform driver data flag.
> 
a big if (driver_data == TWL6032) in each of the ops might be ok, since
we have an int and not a pointer there anyways might be the easiest way
to go.

Regards,
Andreas