[PATCHv4 04/33] CLK: omap: move part of the machine specific clock header contents to driver
Nishanth Menon
nm at ti.com
Tue Jul 30 14:22:20 EDT 2013
this patch should be 3/33 to allow dpll.c to build.
On 07/23/2013 02:19 AM, Tero Kristo wrote:
> Some of the clock.h contents are needed by the new OMAP clock driver,
> including dpll_data and clk_hw_omap. Thus, move these to the generic
> omap header file which can be accessed by the driver.
>
Looking at the change, I wonder what the rules are for the movement?
commit message was not clear.
> Signed-off-by: Tero Kristo <t-kristo at ti.com>
> ---
> arch/arm/mach-omap2/clock.h | 151 +----------------------------------------
> include/linux/clk/omap.h | 157 ++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 157 insertions(+), 151 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h
> index 7aa32cd..d1a3125 100644
> --- a/arch/arm/mach-omap2/clock.h
> +++ b/arch/arm/mach-omap2/clock.h
> @@ -21,6 +21,7 @@
>
> #include <linux/clkdev.h>
> #include <linux/clk-provider.h>
> +#include <linux/clk/omap.h>
>
> struct omap_clk {
> u16 cpu;
> @@ -178,83 +179,6 @@ struct clksel {
> const struct clksel_rate *rates;
> };
>
> -/**
> - * struct dpll_data - DPLL registers and integration data
> - * @mult_div1_reg: register containing the DPLL M and N bitfields
> - * @mult_mask: mask of the DPLL M bitfield in @mult_div1_reg
> - * @div1_mask: mask of the DPLL N bitfield in @mult_div1_reg
> - * @clk_bypass: struct clk pointer to the clock's bypass clock input
> - * @clk_ref: struct clk pointer to the clock's reference clock input
> - * @control_reg: register containing the DPLL mode bitfield
> - * @enable_mask: mask of the DPLL mode bitfield in @control_reg
> - * @last_rounded_rate: cache of the last rate result of omap2_dpll_round_rate()
> - * @last_rounded_m: cache of the last M result of omap2_dpll_round_rate()
> - * @last_rounded_m4xen: cache of the last M4X result of
> - * omap4_dpll_regm4xen_round_rate()
> - * @last_rounded_lpmode: cache of the last lpmode result of
> - * omap4_dpll_lpmode_recalc()
> - * @max_multiplier: maximum valid non-bypass multiplier value (actual)
> - * @last_rounded_n: cache of the last N result of omap2_dpll_round_rate()
> - * @min_divider: minimum valid non-bypass divider value (actual)
> - * @max_divider: maximum valid non-bypass divider value (actual)
> - * @modes: possible values of @enable_mask
> - * @autoidle_reg: register containing the DPLL autoidle mode bitfield
> - * @idlest_reg: register containing the DPLL idle status bitfield
> - * @autoidle_mask: mask of the DPLL autoidle mode bitfield in @autoidle_reg
> - * @freqsel_mask: mask of the DPLL jitter correction bitfield in @control_reg
> - * @idlest_mask: mask of the DPLL idle status bitfield in @idlest_reg
> - * @lpmode_mask: mask of the DPLL low-power mode bitfield in @control_reg
> - * @m4xen_mask: mask of the DPLL M4X multiplier bitfield in @control_reg
> - * @auto_recal_bit: bitshift of the driftguard enable bit in @control_reg
> - * @recal_en_bit: bitshift of the PRM_IRQENABLE_* bit for recalibration IRQs
> - * @recal_st_bit: bitshift of the PRM_IRQSTATUS_* bit for recalibration IRQs
> - * @flags: DPLL type/features (see below)
> - *
> - * Possible values for @flags:
> - * DPLL_J_TYPE: "J-type DPLL" (only some 36xx, 4xxx DPLLs)
> - *
> - * @freqsel_mask is only used on the OMAP34xx family and AM35xx.
> - *
> - * XXX Some DPLLs have multiple bypass inputs, so it's not technically
> - * correct to only have one @clk_bypass pointer.
> - *
> - * XXX The runtime-variable fields (@last_rounded_rate, @last_rounded_m,
> - * @last_rounded_n) should be separated from the runtime-fixed fields
> - * and placed into a different structure, so that the runtime-fixed data
> - * can be placed into read-only space.
> - */
> -struct dpll_data {
> - void __iomem *mult_div1_reg;
> - u32 mult_mask;
> - u32 div1_mask;
> - struct clk *clk_bypass;
> - struct clk *clk_ref;
> - void __iomem *control_reg;
> - u32 enable_mask;
> - unsigned long last_rounded_rate;
> - u16 last_rounded_m;
> - u8 last_rounded_m4xen;
> - u8 last_rounded_lpmode;
> - u16 max_multiplier;
> - u8 last_rounded_n;
> - u8 min_divider;
> - u16 max_divider;
> - u8 modes;
> - void __iomem *autoidle_reg;
> - void __iomem *idlest_reg;
> - u32 autoidle_mask;
> - u32 freqsel_mask;
> - u32 idlest_mask;
> - u32 dco_mask;
> - u32 sddiv_mask;
> - u32 lpmode_mask;
> - u32 m4xen_mask;
> - u8 auto_recal_bit;
> - u8 recal_en_bit;
> - u8 recal_st_bit;
> - u8 flags;
> -};
> -
> /*
> * struct clk.flags possibilities
> *
> @@ -274,56 +198,6 @@ struct dpll_data {
> #define INVERT_ENABLE (1 << 4) /* 0 enables, 1 disables */
> #define CLOCK_CLKOUTX2 (1 << 5)
>
> -/**
> - * struct clk_hw_omap - OMAP struct clk
> - * @node: list_head connecting this clock into the full clock list
> - * @enable_reg: register to write to enable the clock (see @enable_bit)
> - * @enable_bit: bitshift to write to enable/disable the clock (see @enable_reg)
> - * @flags: see "struct clk.flags possibilities" above
> - * @clksel_reg: for clksel clks, register va containing src/divisor select
> - * @clksel_mask: bitmask in @clksel_reg for the src/divisor selector
> - * @clksel: for clksel clks, pointer to struct clksel for this clock
> - * @dpll_data: for DPLLs, pointer to struct dpll_data for this clock
> - * @clkdm_name: clockdomain name that this clock is contained in
> - * @clkdm: pointer to struct clockdomain, resolved from @clkdm_name at runtime
> - * @rate_offset: bitshift for rate selection bitfield (OMAP1 only)
> - * @src_offset: bitshift for source selection bitfield (OMAP1 only)
> - *
> - * XXX @rate_offset, @src_offset should probably be removed and OMAP1
> - * clock code converted to use clksel.
> - *
> - */
> -
> -struct clk_hw_omap_ops;
> -
> -struct clk_hw_omap {
> - struct clk_hw hw;
> - struct list_head node;
> - unsigned long fixed_rate;
> - u8 fixed_div;
> - void __iomem *enable_reg;
> - u8 enable_bit;
> - u8 flags;
> - void __iomem *clksel_reg;
> - u32 clksel_mask;
> - const struct clksel *clksel;
> - struct dpll_data *dpll_data;
> - const char *clkdm_name;
> - struct clockdomain *clkdm;
> - const struct clk_hw_omap_ops *ops;
> -};
> -
> -struct clk_hw_omap_ops {
> - void (*find_idlest)(struct clk_hw_omap *oclk,
> - void __iomem **idlest_reg,
> - u8 *idlest_bit, u8 *idlest_val);
> - void (*find_companion)(struct clk_hw_omap *oclk,
> - void __iomem **other_reg,
> - u8 *other_bit);
> - void (*allow_idle)(struct clk_hw_omap *oclk);
> - void (*deny_idle)(struct clk_hw_omap *oclk);
> -};
> -
> unsigned long omap_fixed_divisor_recalc(struct clk_hw *hw,
> unsigned long parent_rate);
>
> @@ -356,28 +230,13 @@ unsigned long omap_fixed_divisor_recalc(struct clk_hw *hw,
> /* DPLL Type and DCO Selection Flags */
> #define DPLL_J_TYPE 0x1
>
> -long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
> - unsigned long *parent_rate);
> -unsigned long omap3_dpll_recalc(struct clk_hw *hw, unsigned long parent_rate);
> -int omap3_noncore_dpll_enable(struct clk_hw *hw);
> -void omap3_noncore_dpll_disable(struct clk_hw *hw);
> -int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> - unsigned long parent_rate);
Why are these being moved to generic?
> u32 omap3_dpll_autoidle_read(struct clk_hw_omap *clk);
> void omap3_dpll_allow_idle(struct clk_hw_omap *clk);
> void omap3_dpll_deny_idle(struct clk_hw_omap *clk);
> -unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> - unsigned long parent_rate);
why move this?
> int omap4_dpllmx_gatectrl_read(struct clk_hw_omap *clk);
> void omap4_dpllmx_allow_gatectrl(struct clk_hw_omap *clk);
> void omap4_dpllmx_deny_gatectrl(struct clk_hw_omap *clk);
> -unsigned long omap4_dpll_regm4xen_recalc(struct clk_hw *hw,
> - unsigned long parent_rate);
> -long omap4_dpll_regm4xen_round_rate(struct clk_hw *hw,
> - unsigned long target_rate,
> - unsigned long *parent_rate);
same here.
>
> -void omap2_init_clk_clkdm(struct clk_hw *clk);
same question again.
> void __init omap2_clk_disable_clkdm_control(void);
>
> /* clkt_clksel.c public functions */
> @@ -396,7 +255,6 @@ int omap2_clksel_set_parent(struct clk_hw *hw, u8 field_val);
> extern void omap2_clkt_iclk_allow_idle(struct clk_hw_omap *clk);
> extern void omap2_clkt_iclk_deny_idle(struct clk_hw_omap *clk);
>
> -u8 omap2_init_dpll_parent(struct clk_hw *hw);
why move this?
> unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk);
>
> int omap2_dflt_clk_enable(struct clk_hw *hw);
> @@ -408,9 +266,7 @@ void omap2_clk_dflt_find_companion(struct clk_hw_omap *clk,
> void omap2_clk_dflt_find_idlest(struct clk_hw_omap *clk,
> void __iomem **idlest_reg,
> u8 *idlest_bit, u8 *idlest_val);
> -void omap2_init_clk_hw_omap_clocks(struct clk *clk);
why move this?
> int omap2_clk_enable_autoidle_all(void);
> -int omap2_clk_disable_autoidle_all(void);
why move this?
> void omap2_clk_enable_init_clocks(const char **clk_names, u8 num_clocks);
> int omap2_clk_switch_mpurate_at_boot(const char *mpurate_ck_name);
> void omap2_clk_print_new_rates(const char *hfclkin_ck_name,
> @@ -431,10 +287,8 @@ extern const struct clksel_rate gfx_l3_rates[];
> extern const struct clksel_rate dsp_ick_rates[];
> extern struct clk dummy_ck;
>
> -extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
is this needed to be moved?
> extern const struct clk_hw_omap_ops clkhwops_iclk_wait;
> extern const struct clk_hw_omap_ops clkhwops_wait;
> -extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;
is this needed to be moved?
> extern const struct clk_hw_omap_ops clkhwops_iclk;
> extern const struct clk_hw_omap_ops clkhwops_omap3430es2_ssi_wait;
> extern const struct clk_hw_omap_ops clkhwops_omap3430es2_iclk_ssi_wait;
> @@ -460,8 +314,5 @@ extern const struct clksel_rate div31_1to31_rates[];
>
> extern int am33xx_clk_init(void);
>
> -extern int omap2_clkops_enable_clkdm(struct clk_hw *hw);
> -extern void omap2_clkops_disable_clkdm(struct clk_hw *hw);
> -
> extern void omap_clocks_register(struct omap_clk *oclks, int cnt);
> #endif
> diff --git a/include/linux/clk/omap.h b/include/linux/clk/omap.h
> index 647f02f..cba892a 100644
> --- a/include/linux/clk/omap.h
> +++ b/include/linux/clk/omap.h
> @@ -19,6 +19,161 @@
> #ifndef __LINUX_CLK_OMAP_H_
> #define __LINUX_CLK_OMAP_H_
>
> -int __init dt_omap_clk_init(void);
> +/**
> + * struct dpll_data - DPLL registers and integration data
> + * @mult_div1_reg: register containing the DPLL M and N bitfields
> + * @mult_mask: mask of the DPLL M bitfield in @mult_div1_reg
> + * @div1_mask: mask of the DPLL N bitfield in @mult_div1_reg
> + * @clk_bypass: struct clk pointer to the clock's bypass clock input
> + * @clk_ref: struct clk pointer to the clock's reference clock input
> + * @control_reg: register containing the DPLL mode bitfield
> + * @enable_mask: mask of the DPLL mode bitfield in @control_reg
> + * @last_rounded_rate: cache of the last rate result of omap2_dpll_round_rate()
> + * @last_rounded_m: cache of the last M result of omap2_dpll_round_rate()
> + * @last_rounded_m4xen: cache of the last M4X result of
> + * omap4_dpll_regm4xen_round_rate()
> + * @last_rounded_lpmode: cache of the last lpmode result of
> + * omap4_dpll_lpmode_recalc()
> + * @max_multiplier: maximum valid non-bypass multiplier value (actual)
> + * @last_rounded_n: cache of the last N result of omap2_dpll_round_rate()
> + * @min_divider: minimum valid non-bypass divider value (actual)
> + * @max_divider: maximum valid non-bypass divider value (actual)
> + * @modes: possible values of @enable_mask
> + * @autoidle_reg: register containing the DPLL autoidle mode bitfield
> + * @idlest_reg: register containing the DPLL idle status bitfield
> + * @autoidle_mask: mask of the DPLL autoidle mode bitfield in @autoidle_reg
> + * @freqsel_mask: mask of the DPLL jitter correction bitfield in @control_reg
> + * @idlest_mask: mask of the DPLL idle status bitfield in @idlest_reg
> + * @lpmode_mask: mask of the DPLL low-power mode bitfield in @control_reg
> + * @m4xen_mask: mask of the DPLL M4X multiplier bitfield in @control_reg
> + * @auto_recal_bit: bitshift of the driftguard enable bit in @control_reg
> + * @recal_en_bit: bitshift of the PRM_IRQENABLE_* bit for recalibration IRQs
> + * @recal_st_bit: bitshift of the PRM_IRQSTATUS_* bit for recalibration IRQs
> + * @flags: DPLL type/features (see below)
> + *
> + * Possible values for @flags:
> + * DPLL_J_TYPE: "J-type DPLL" (only some 36xx, 4xxx DPLLs)
but the flag is in arch/arm/mach-omap2/clock.h ?
> + *
> + * @freqsel_mask is only used on the OMAP34xx family and AM35xx.
> + *
> + * XXX Some DPLLs have multiple bypass inputs, so it's not technically
> + * correct to only have one @clk_bypass pointer.
> + *
> + * XXX The runtime-variable fields (@last_rounded_rate, @last_rounded_m,
> + * @last_rounded_n) should be separated from the runtime-fixed fields
> + * and placed into a different structure, so that the runtime-fixed data
> + * can be placed into read-only space.
> + */
> +struct dpll_data {
is it necessary to export this? usage is limited to dpll.c and could be
made private to it alone, no?
> + void __iomem *mult_div1_reg;
> + u32 mult_mask;
> + u32 div1_mask;
> + struct clk *clk_bypass;
> + struct clk *clk_ref;
> + void __iomem *control_reg;
> + u32 enable_mask;
> + unsigned long last_rounded_rate;
> + u16 last_rounded_m;
> + u8 last_rounded_m4xen;
> + u8 last_rounded_lpmode;
> + u16 max_multiplier;
> + u8 last_rounded_n;
> + u8 min_divider;
> + u16 max_divider;
> + u8 modes;
> + void __iomem *autoidle_reg;
> + void __iomem *idlest_reg;
> + u32 autoidle_mask;
> + u32 freqsel_mask;
> + u32 idlest_mask;
> + u32 dco_mask;
not part of kernel documentation above.
> + u32 sddiv_mask;
not part of kernel documentation above.
> + u32 lpmode_mask;
> + u32 m4xen_mask;
> + u8 auto_recal_bit;
> + u8 recal_en_bit;
> + u8 recal_st_bit;
> + u8 flags;
> +};
> +
> +/**
> + * struct clk_hw_omap - OMAP struct clk
> + * @node: list_head connecting this clock into the full clock list
> + * @enable_reg: register to write to enable the clock (see @enable_bit)
> + * @enable_bit: bitshift to write to enable/disable the clock (see @enable_reg)
> + * @flags: see "struct clk.flags possibilities" above
> + * @clksel_reg: for clksel clks, register va containing src/divisor select
> + * @clksel_mask: bitmask in @clksel_reg for the src/divisor selector
> + * @clksel: for clksel clks, pointer to struct clksel for this clock
> + * @dpll_data: for DPLLs, pointer to struct dpll_data for this clock
> + * @clkdm_name: clockdomain name that this clock is contained in
> + * @clkdm: pointer to struct clockdomain, resolved from @clkdm_name at runtime
> + * @rate_offset: bitshift for rate selection bitfield (OMAP1 only)
> + * @src_offset: bitshift for source selection bitfield (OMAP1 only)
> + *
> + * XXX @rate_offset, @src_offset should probably be removed and OMAP1
> + * clock code converted to use clksel.
Do you need to carry these forward? they already disappeared from the
struct below.
> + *
> + */
> +
> +struct clk_hw_omap_ops;
you need to keep the kernel documentation next to the struct which it is
documenting, by introducing clk_hw_omap_ops forward declaration, we
introduced the following kernel-doc error:
Error(include/linux/clk/omap.h:119): Cannot parse struct or union!
> +
> +struct clk_hw_omap {
> + struct clk_hw hw;
not documented.
> + struct list_head node;
> + unsigned long fixed_rate;
not documented.
> + u8 fixed_div;
not documented.
> + void __iomem *enable_reg;
> + u8 enable_bit;
> + u8 flags;
> + void __iomem *clksel_reg;
> + u32 clksel_mask;
> + const struct clksel *clksel;
> + struct dpll_data *dpll_data;
> + const char *clkdm_name;
> + struct clockdomain *clkdm;
> + const struct clk_hw_omap_ops *ops;
> +};
> +
> +struct clk_hw_omap_ops {
> + void (*find_idlest)(struct clk_hw_omap *oclk,
> + void __iomem **idlest_reg,
> + u8 *idlest_bit, u8 *idlest_val);
> + void (*find_companion)(struct clk_hw_omap *oclk,
> + void __iomem **other_reg,
> + u8 *other_bit);
> + void (*allow_idle)(struct clk_hw_omap *oclk);
> + void (*deny_idle)(struct clk_hw_omap *oclk);
> +};
will be nice to have kernel documentation for these.
> +
> +void omap2_init_clk_hw_omap_clocks(struct clk *clk);
> +int omap3_noncore_dpll_enable(struct clk_hw *hw);
> +void omap3_noncore_dpll_disable(struct clk_hw *hw);
> +int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate);
> +unsigned long omap4_dpll_regm4xen_recalc(struct clk_hw *hw,
> + unsigned long parent_rate);
> +long omap4_dpll_regm4xen_round_rate(struct clk_hw *hw,
> + unsigned long target_rate,
> + unsigned long *parent_rate);
> +u8 omap2_init_dpll_parent(struct clk_hw *hw);
> +unsigned long omap3_dpll_recalc(struct clk_hw *hw, unsigned long parent_rate);
> +long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
> + unsigned long *parent_rate);
> +void omap2_init_clk_clkdm(struct clk_hw *clk);
> +unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> + unsigned long parent_rate);
> +
> +int omap2_clkops_enable_clkdm(struct clk_hw *hw);
> +void omap2_clkops_disable_clkdm(struct clk_hw *hw);
> +
> +int omap2_clk_disable_autoidle_all(void);
^^ all the above, not clear why we are moving them out enmasse.
> +
> +extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
> +extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;
why do we need to export these out?
> +
> +/* DT functions */
> +int dt_omap_clk_init(void);
in this change, we removed __init -> which should have been done in the
patch that introduced it in the first place.
> +void of_omap4_dpll_setup(struct device_node *node);
we should not be needing this.
>
> #endif
>
at this point, we have a dependency between drivers/clk/omap/dpll.c and
arch/arm/mach-omap2/ -> a possible suggestion will be to move required
data structures first and operations as required.
--
Regards,
Nishanth Menon
More information about the linux-arm-kernel
mailing list