| Message ID | 20240924103609.12513-3-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | mfd: twl: Add clock for TWL6030 | expand |
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 */ > }
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
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 */ }
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(-)