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

[Roger Quadros <rogerq@kernel.org>]

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