diff for duplicates of <20130805171122.5348.59628@quantum> diff --git a/a/1.txt b/N1/1.txt index 28b7d57..9b5e14b 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -2,76 +2,57 @@ Quoting Gerhard Sittig (2013-08-03 07:39:56) > [ we are strictly talking about clocks and source code again, > I have trimmed the CC: list to not spam the device tree ML or > subsystem maintainers ] -> = - +> > On Fri, Aug 02, 2013 at 16:30 -0700, Mike Turquette wrote: -> > = - +> > > > Quoting Gerhard Sittig (2013-07-23 06:14:06) > > > [ summary: "shared gate" support desirable? approach acceptable? ] -> > > = - +> > > > > > On Mon, Jul 22, 2013 at 14:14 +0200, Gerhard Sittig wrote: -> > > > = - -> > > > this change implements a clock driver for the MPC512x PowerPC platf= -orm +> > > > +> > > > this change implements a clock driver for the MPC512x PowerPC platform > > > > which follows the COMMON_CLK approach and uses common clock drivers > > > > shared with other platforms -> > > > = - +> > > > > > > > [ ... ] -> > > > = - -> > > > some of the clock items get pre-enabled in the clock driver to not = -have -> > > > them automatically disabled by the underlying clock subsystem becau= -se of +> > > > +> > > > some of the clock items get pre-enabled in the clock driver to not have +> > > > them automatically disabled by the underlying clock subsystem because of > > > > their being unused -- this approach is desirable because > > > > [ ... ] > > > > - some help introduce support for and migrate to the common -> > > > infrastructure, while more appropriate support for specific hardw= -are +> > > > infrastructure, while more appropriate support for specific hardware > > > > constraints isn't available yet (remaining changes are strictly > > > > internal to the clock driver and won't affect peripheral drivers) -> > > = - +> > > > > > This remark was related to the CAN clocks of the MPC512x SoC. -> > = - +> > > > Gerhard, -> > = - +> > > > Thanks for the patch (way far down below here). I'll check into it to > > see if that implementation looks OK. It would be helpful if another > > platform with shared gates could weigh in on whether the implementation > > works for them. -> > = - +> > > > Still, a shared gate solution is not a prerequisite for this series, > > correct? -> = - +> > Well, the recent CAN driver related discussion suggested that I > had a mental misconception there. The need for "shared gates" > was felt because of mixing up unrelated paths in the clock tree. > But the MCLK subtree is for bitrate generation, while the BDLC > gate is for register access into the peripheral controller. -> = - +> > Currently I'm investigating how I can cleanly tell those > individual aspects apart. Telling the gate for register access > (in ARM speak often referred to as 'ipg') from the bitrate > generation (the 'per' clock, or 'mclk' here) seems so much more > appropriate. -> = - +> > After clean separation, and more testing to make sure nothing > gets broken throughout the series, there will be v4. -> = - -> = - +> +> > So "shared gate" support might have become obsolete for the > MPC512x platform. But if others need it, the outlined approach > (patch below) may be viable. The change to the common code is @@ -79,8 +60,7 @@ are > overengineered for the case of exactly one such gate, but this > immediately makes it a working approach for several gates, if > others need it. -> = - +> > I'll trim the motivation and just leave the suggested approach > for "shared gates" here. Feel free to drop it or to only > resurrect it as the need may re-arise later. So far nobody @@ -93,38 +73,31 @@ more time on it. Regards, Mike -> = - +> > > > [ ... ] -> > > = - +> > > > > > The question now is how to correctly support the situation where > > > a gate is shared between subtrees yet isn't really part of any > > > path within the subtrees. I really cannot find a single spot > > > where to introduce the gate such that it's not duplicated. -> > > = - +> > > > > > The appropriate solution would not be to pre-enable those clocks, > > > but to either introduce another gate clock type which supports a > > > shared reference, or to add support for the shared reference to > > > the existing gate code. -> > > = - -> > > = - +> > > +> > > > > > I'd rather not duplicate most or all of the code of clk-gate.c, > > > instead I looked into how to add "shared gate" support to the > > > existing driver. -> > > = - +> > > > > > My question is whether the approach is acceptable. It adds > > > minimal overhead and shall be OK for the enable/disable path from > > > a technical POV. And it doesn't feel like too much of a stretch. > > > But there may be non-technical reasons to reject the approach. > > > I'd like to learn whether to follow that path before preparing > > > another version of the patch series. -> > > = - +> > > > > > The diffs were taken with the '-w -b' options to demonstrate > > > their essence and not drown it in whitespace changes. The > > > implementation assumes that the caller which registers the gate @@ -132,8 +105,7 @@ Mike > > > the lock. And that all gates with a "shared use counter" use the > > > same lock (which is satisfied as they all get registered from the > > > same spot in the platform's clock driver). -> > > = - +> > > > > > The CLK_IGNORE_UNUSED flag addresses a different problem. The > > > SoC has four MSCAN components, while two of them are enabled in > > > the device tree (the other two are present but disabled). So @@ -145,65 +117,52 @@ Mike > > > the common gate logic would implement a separate disable_unused() > > > routine, but I guess this isn't necessary and the use of the flag > > > is appropriate. -> > > = - +> > > > > > That the example use creates a field for just one counter is to > > > better demonstrate the use and potential extension as need > > > arises. Reducing this to a mere integer variable would be a > > > micro optimization. -> > > = - -> > > = - +> > > +> > > > > > The extension of the existing clk_gate implementation: -> > > = - +> > > > > > --- a/drivers/clk/clk-gate.c > > > +++ b/drivers/clk/clk-gate.c -> > > @@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, i= -nt enable) -> > > struct clk_gate *gate =3D to_clk_gate(hw); -> > > int set =3D gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0; -> > > unsigned long flags =3D 0; +> > > @@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) +> > > struct clk_gate *gate = to_clk_gate(hw); +> > > int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0; +> > > unsigned long flags = 0; > > > + int need_reg_access; > > > u32 reg; -> > > = - -> > > set ^=3D enable; -> > > @@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, = -int enable) +> > > +> > > set ^= enable; +> > > @@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) > > > if (gate->lock) > > > spin_lock_irqsave(gate->lock, flags); -> > > = - +> > > > > > + /* -> > > + * if a "shared use counter" was specified, keep track of ena= -ble -> > > + * and disable calls and only access hardware registers upon = -the +> > > + * if a "shared use counter" was specified, keep track of enable +> > > + * and disable calls and only access hardware registers upon the > > > + * very first enable or very last disable call > > > + */ > > > + if (!gate->share_count) { -> > > + need_reg_access =3D 1; +> > > + need_reg_access = 1; > > > + } else if (enable) { -> > > + need_reg_access =3D (*gate->share_count)++ =3D=3D 0; +> > > + need_reg_access = (*gate->share_count)++ == 0; > > > + } else { -> > > + need_reg_access =3D --(*gate->share_count) =3D=3D 0; +> > > + need_reg_access = --(*gate->share_count) == 0; > > > + } > > > + > > > + if (need_reg_access) { > > > if (gate->flags & CLK_GATE_HIWORD_MASK) { -> > > reg =3D BIT(gate->bit_idx + 16); +> > > reg = BIT(gate->bit_idx + 16); > > > if (set) -> > > @@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, i= -nt enable) +> > > @@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) > > > } -> > > = - +> > > > > > clk_writel(reg, gate->reg); > > > + } -> > > = - +> > > > > > if (gate->lock) > > > spin_unlock_irqrestore(gate->lock, flags); > > > @@ -118,10 +134,11 @@ EXPORT_SYMBOL_GPL(clk_gate_ops); @@ -211,8 +170,7 @@ nt enable) > > > * @lock: shared register lock for this clock > > > */ > > > -struct clk *clk_register_gate(struct device *dev, const char *name, -> > > +struct clk *clk_register_gate_shared(struct device *dev, const char = -*name, +> > > +struct clk *clk_register_gate_shared(struct device *dev, const char *name, > > > const char *parent_name, unsigned long flags, > > > void __iomem *reg, u8 bit_idx, > > > - u8 clk_gate_flags, spinlock_t *lock) @@ -221,20 +179,16 @@ nt enable) > > > { > > > struct clk_gate *gate; > > > struct clk *clk; -> > > @@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev,= - const char *name, -> > > gate->bit_idx =3D bit_idx; -> > > gate->flags =3D clk_gate_flags; -> > > gate->lock =3D lock; -> > > + gate->share_count =3D share_count; -> > > gate->hw.init =3D &init; -> > > = - -> > > clk =3D clk_register(dev, &gate->hw); -> > > @@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev= -, const char *name, -> > > = - +> > > @@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name, +> > > gate->bit_idx = bit_idx; +> > > gate->flags = clk_gate_flags; +> > > gate->lock = lock; +> > > + gate->share_count = share_count; +> > > gate->hw.init = &init; +> > > +> > > clk = clk_register(dev, &gate->hw); +> > > @@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev, const char *name, +> > > > > > return clk; > > > } > > > + @@ -256,38 +210,30 @@ nt enable) > > > spinlock_t *lock; > > > + int *share_count; > > > }; -> > > = - +> > > > > > #define CLK_GATE_SET_TO_DISABLE BIT(0) -> > > @@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev= -, const char *name, +> > > @@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev, const char *name, > > > const char *parent_name, unsigned long flags, > > > void __iomem *reg, u8 bit_idx, > > > u8 clk_gate_flags, spinlock_t *lock); -> > > +struct clk *clk_register_gate_shared(struct device *dev, const char = -*name, +> > > +struct clk *clk_register_gate_shared(struct device *dev, const char *name, > > > + const char *parent_name, unsigned long flags, > > > + void __iomem *reg, u8 bit_idx, > > > + u8 clk_gate_flags, spinlock_t *lock, > > > + int *share_count); -> > > = - +> > > > > > struct clk_div_table { > > > unsigned int val; -> > > = - -> > > = - +> > > +> > > > > > How to use these shared gates: -> > > = - +> > > > > > --- a/arch/powerpc/platforms/512x/clock-commonclk.c > > > +++ b/arch/powerpc/platforms/512x/clock-commonclk.c > > > @@ -123,6 +123,39 @@ static inline struct clk *mpc512x_clk_gated( > > > reg, pos, 0, &clklock); > > > } -> > > = - +> > > > > > +enum mpc512x_clk_shared_gate_id_t { > > > + MPC512x_CLK_SHARED_GATE_MSCAN, > > > + MPC512x_CLK_SHARED_GATE_MAX, @@ -296,21 +242,15 @@ nt enable) > > > +static int mpc512x_clk_gate_counters[MPC512x_CLK_SHARED_GATE_MAX]; > > > + > > > +/* -> > > + * implementor's note: since clk_gate items don't implement a separ= -ate -> > > + * .disable_unused() callback, their .disable() routine gets called = -and -> > > + * "disable the clock as we can't see it's in use" cannot be told fr= -om +> > > + * implementor's note: since clk_gate items don't implement a separate +> > > + * .disable_unused() callback, their .disable() routine gets called and +> > > + * "disable the clock as we can't see it's in use" cannot be told from > > > + * "regular disable, count these events please" > > > + * -> > > + * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppr= -ess -> > > + * the "disable, unused" call, so use counts won't get unbalanced, t= -he +> > > + * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppress +> > > + * the "disable, unused" call, so use counts won't get unbalanced, the > > > + * clock either never got enabled and thus need not get disabled, or -> > > + * part of the hardware got enabled while disabling the other part i= -sn't +> > > + * part of the hardware got enabled while disabling the other part isn't > > > + * wanted > > > + */ > > > +static inline struct clk *mpc512x_clk_gated_shared( @@ -320,49 +260,40 @@ sn't > > > +{ > > > + int clkflags; > > > + -> > > + clkflags =3D CLK_SET_RATE_PARENT; -> > > + clkflags |=3D CLK_IGNORE_UNUSED; -> > > + return clk_register_gate_shared(NULL, name, parent_name, clkf= -lags, +> > > + clkflags = CLK_SET_RATE_PARENT; +> > > + clkflags |= CLK_IGNORE_UNUSED; +> > > + return clk_register_gate_shared(NULL, name, parent_name, clkflags, > > > + reg, pos, 0, &clklock, -> > > + &mpc512x_clk_gate_counters[sh= -are_id]); +> > > + &mpc512x_clk_gate_counters[share_id]); > > > +} > > > + > > > static inline struct clk *mpc512x_clk_muxed(const char *name, > > > const char **parent_names, int parent_count, > > > u32 __iomem *reg, u8 pos, u8 len) -> > > @@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_s= -etup_data *entry) +> > > @@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry) > > > 1, 1); > > > } > > > if (sccr_reg) { -> > > + if (entry->type =3D=3D MCLK_TYPE_MSCAN) { -> > > + clks[clks_idx_pub] =3D mpc512x_clk_gated_shar= -ed( +> > > + if (entry->type == MCLK_TYPE_MSCAN) { +> > > + clks[clks_idx_pub] = mpc512x_clk_gated_shared( > > > + entry->name_mclk, -> > > + entry->name_mux1, sccr_reg, s= -ccr_bit, -> > > + MPC512x_CLK_SHARED_GATE_MSCAN= -); +> > > + entry->name_mux1, sccr_reg, sccr_bit, +> > > + MPC512x_CLK_SHARED_GATE_MSCAN); > > > + } else { -> > > clks[clks_idx_pub] =3D mpc512x_clk_gated( +> > > clks[clks_idx_pub] = mpc512x_clk_gated( > > > entry->name_mclk, -> > > entry->name_mux1, sccr_reg, s= -ccr_bit); +> > > entry->name_mux1, sccr_reg, sccr_bit); > > > + } > > > } else { -> > > clks[clks_idx_pub] =3D mpc512x_clk_factor( +> > > clks[clks_idx_pub] = mpc512x_clk_factor( > > > entry->name_mclk, -> > > = - +> > > > > > Local tests have shown that the extension solves the problem of > > > how to satisfy the SoC's constraints on the MPC512x platform. > > > The MSCAN clocks no longer need to get pre-enabled, instead they > > > get setup and enabled only as the mscan(4) driver probes devices > > > according to how it was instructed (device tree nodes). -> > > = - +> > > > > > What do you think? Is the "shared gate" support in the common > > > logic appropriate? I'd rather not duplicate all of this code > > > just to introduce the specific gate I need, while most of the @@ -370,14 +301,11 @@ ccr_bit); > > > desire isn't to override the gate's operations, but to wrap them > > > and to consult a counter in addition, while the register access > > > still applies. -> = - -> = - +> +> > virtually yours > Gerhard Sittig -> -- = - +> -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany -> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de +> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de diff --git a/a/content_digest b/N1/content_digest index 99a2acd..59fee3c 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -4,90 +4,67 @@ "ref\020130723131406.GI19071@book.gsilab.sittig.org\0" "ref\020130802233009.6450.21915@quantum\0" "ref\020130803143956.GH2580@book.gsilab.sittig.org\0" - "From\0Mike Turquette <mturquette@linaro.org>\0" - "Subject\0Re: [PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x\0" + "From\0mturquette@linaro.org (Mike Turquette)\0" + "Subject\0[PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x\0" "Date\0Mon, 05 Aug 2013 10:11:22 -0700\0" - "To\0Gerhard Sittig <gsi@denx.de>" - "\0" - "Cc\0Anatolij Gustschin <agust@denx.de>" - linuxppc-dev@lists.ozlabs.org - " linux-arm-kernel@lists.infradead.org\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "Quoting Gerhard Sittig (2013-08-03 07:39:56)\n" "> [ we are strictly talking about clocks and source code again,\n" "> I have trimmed the CC: list to not spam the device tree ML or\n" "> subsystem maintainers ]\n" - "> =\n" - "\n" + "> \n" "> On Fri, Aug 02, 2013 at 16:30 -0700, Mike Turquette wrote:\n" - "> > =\n" - "\n" + "> > \n" "> > Quoting Gerhard Sittig (2013-07-23 06:14:06)\n" "> > > [ summary: \"shared gate\" support desirable? approach acceptable? ]\n" - "> > > =\n" - "\n" + "> > > \n" "> > > On Mon, Jul 22, 2013 at 14:14 +0200, Gerhard Sittig wrote:\n" - "> > > > =\n" - "\n" - "> > > > this change implements a clock driver for the MPC512x PowerPC platf=\n" - "orm\n" + "> > > > \n" + "> > > > this change implements a clock driver for the MPC512x PowerPC platform\n" "> > > > which follows the COMMON_CLK approach and uses common clock drivers\n" "> > > > shared with other platforms\n" - "> > > > =\n" - "\n" + "> > > > \n" "> > > > [ ... ]\n" - "> > > > =\n" - "\n" - "> > > > some of the clock items get pre-enabled in the clock driver to not =\n" - "have\n" - "> > > > them automatically disabled by the underlying clock subsystem becau=\n" - "se of\n" + "> > > > \n" + "> > > > some of the clock items get pre-enabled in the clock driver to not have\n" + "> > > > them automatically disabled by the underlying clock subsystem because of\n" "> > > > their being unused -- this approach is desirable because\n" "> > > > [ ... ]\n" "> > > > - some help introduce support for and migrate to the common\n" - "> > > > infrastructure, while more appropriate support for specific hardw=\n" - "are\n" + "> > > > infrastructure, while more appropriate support for specific hardware\n" "> > > > constraints isn't available yet (remaining changes are strictly\n" "> > > > internal to the clock driver and won't affect peripheral drivers)\n" - "> > > =\n" - "\n" + "> > > \n" "> > > This remark was related to the CAN clocks of the MPC512x SoC.\n" - "> > =\n" - "\n" + "> > \n" "> > Gerhard,\n" - "> > =\n" - "\n" + "> > \n" "> > Thanks for the patch (way far down below here). I'll check into it to\n" "> > see if that implementation looks OK. It would be helpful if another\n" "> > platform with shared gates could weigh in on whether the implementation\n" "> > works for them.\n" - "> > =\n" - "\n" + "> > \n" "> > Still, a shared gate solution is not a prerequisite for this series,\n" "> > correct?\n" - "> =\n" - "\n" + "> \n" "> Well, the recent CAN driver related discussion suggested that I\n" "> had a mental misconception there. The need for \"shared gates\"\n" "> was felt because of mixing up unrelated paths in the clock tree.\n" "> But the MCLK subtree is for bitrate generation, while the BDLC\n" "> gate is for register access into the peripheral controller.\n" - "> =\n" - "\n" + "> \n" "> Currently I'm investigating how I can cleanly tell those\n" "> individual aspects apart. Telling the gate for register access\n" "> (in ARM speak often referred to as 'ipg') from the bitrate\n" "> generation (the 'per' clock, or 'mclk' here) seems so much more\n" "> appropriate.\n" - "> =\n" - "\n" + "> \n" "> After clean separation, and more testing to make sure nothing\n" "> gets broken throughout the series, there will be v4.\n" - "> =\n" - "\n" - "> =\n" - "\n" + "> \n" + "> \n" "> So \"shared gate\" support might have become obsolete for the\n" "> MPC512x platform. But if others need it, the outlined approach\n" "> (patch below) may be viable. The change to the common code is\n" @@ -95,8 +72,7 @@ "> overengineered for the case of exactly one such gate, but this\n" "> immediately makes it a working approach for several gates, if\n" "> others need it.\n" - "> =\n" - "\n" + "> \n" "> I'll trim the motivation and just leave the suggested approach\n" "> for \"shared gates\" here. Feel free to drop it or to only\n" "> resurrect it as the need may re-arise later. So far nobody\n" @@ -109,38 +85,31 @@ "Regards,\n" "Mike\n" "\n" - "> =\n" - "\n" + "> \n" "> > > [ ... ]\n" - "> > > =\n" - "\n" + "> > > \n" "> > > The question now is how to correctly support the situation where\n" "> > > a gate is shared between subtrees yet isn't really part of any\n" "> > > path within the subtrees. I really cannot find a single spot\n" "> > > where to introduce the gate such that it's not duplicated.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > The appropriate solution would not be to pre-enable those clocks,\n" "> > > but to either introduce another gate clock type which supports a\n" "> > > shared reference, or to add support for the shared reference to\n" "> > > the existing gate code.\n" - "> > > =\n" - "\n" - "> > > =\n" - "\n" + "> > > \n" + "> > > \n" "> > > I'd rather not duplicate most or all of the code of clk-gate.c,\n" "> > > instead I looked into how to add \"shared gate\" support to the\n" "> > > existing driver.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > My question is whether the approach is acceptable. It adds\n" "> > > minimal overhead and shall be OK for the enable/disable path from\n" "> > > a technical POV. And it doesn't feel like too much of a stretch.\n" "> > > But there may be non-technical reasons to reject the approach.\n" "> > > I'd like to learn whether to follow that path before preparing\n" "> > > another version of the patch series.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > The diffs were taken with the '-w -b' options to demonstrate\n" "> > > their essence and not drown it in whitespace changes. The\n" "> > > implementation assumes that the caller which registers the gate\n" @@ -148,8 +117,7 @@ "> > > the lock. And that all gates with a \"shared use counter\" use the\n" "> > > same lock (which is satisfied as they all get registered from the\n" "> > > same spot in the platform's clock driver).\n" - "> > > =\n" - "\n" + "> > > \n" "> > > The CLK_IGNORE_UNUSED flag addresses a different problem. The\n" "> > > SoC has four MSCAN components, while two of them are enabled in\n" "> > > the device tree (the other two are present but disabled). So\n" @@ -161,65 +129,52 @@ "> > > the common gate logic would implement a separate disable_unused()\n" "> > > routine, but I guess this isn't necessary and the use of the flag\n" "> > > is appropriate.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > That the example use creates a field for just one counter is to\n" "> > > better demonstrate the use and potential extension as need\n" "> > > arises. Reducing this to a mere integer variable would be a\n" "> > > micro optimization.\n" - "> > > =\n" - "\n" - "> > > =\n" - "\n" + "> > > \n" + "> > > \n" "> > > The extension of the existing clk_gate implementation:\n" - "> > > =\n" - "\n" + "> > > \n" "> > > --- a/drivers/clk/clk-gate.c\n" "> > > +++ b/drivers/clk/clk-gate.c\n" - "> > > @@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, i=\n" - "nt enable)\n" - "> > > struct clk_gate *gate =3D to_clk_gate(hw);\n" - "> > > int set =3D gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;\n" - "> > > unsigned long flags =3D 0;\n" + "> > > @@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)\n" + "> > > struct clk_gate *gate = to_clk_gate(hw);\n" + "> > > int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;\n" + "> > > unsigned long flags = 0;\n" "> > > + int need_reg_access;\n" "> > > u32 reg;\n" - "> > > =\n" - "\n" - "> > > set ^=3D enable;\n" - "> > > @@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, =\n" - "int enable)\n" + "> > > \n" + "> > > set ^= enable;\n" + "> > > @@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)\n" "> > > if (gate->lock)\n" "> > > spin_lock_irqsave(gate->lock, flags);\n" - "> > > =\n" - "\n" + "> > > \n" "> > > + /*\n" - "> > > + * if a \"shared use counter\" was specified, keep track of ena=\n" - "ble\n" - "> > > + * and disable calls and only access hardware registers upon =\n" - "the\n" + "> > > + * if a \"shared use counter\" was specified, keep track of enable\n" + "> > > + * and disable calls and only access hardware registers upon the\n" "> > > + * very first enable or very last disable call\n" "> > > + */\n" "> > > + if (!gate->share_count) {\n" - "> > > + need_reg_access =3D 1;\n" + "> > > + need_reg_access = 1;\n" "> > > + } else if (enable) {\n" - "> > > + need_reg_access =3D (*gate->share_count)++ =3D=3D 0;\n" + "> > > + need_reg_access = (*gate->share_count)++ == 0;\n" "> > > + } else {\n" - "> > > + need_reg_access =3D --(*gate->share_count) =3D=3D 0;\n" + "> > > + need_reg_access = --(*gate->share_count) == 0;\n" "> > > + }\n" "> > > +\n" "> > > + if (need_reg_access) {\n" "> > > if (gate->flags & CLK_GATE_HIWORD_MASK) {\n" - "> > > reg =3D BIT(gate->bit_idx + 16);\n" + "> > > reg = BIT(gate->bit_idx + 16);\n" "> > > if (set)\n" - "> > > @@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, i=\n" - "nt enable)\n" + "> > > @@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)\n" "> > > }\n" - "> > > =\n" - "\n" + "> > > \n" "> > > clk_writel(reg, gate->reg);\n" "> > > + }\n" - "> > > =\n" - "\n" + "> > > \n" "> > > if (gate->lock)\n" "> > > spin_unlock_irqrestore(gate->lock, flags);\n" "> > > @@ -118,10 +134,11 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);\n" @@ -227,8 +182,7 @@ "> > > * @lock: shared register lock for this clock\n" "> > > */\n" "> > > -struct clk *clk_register_gate(struct device *dev, const char *name,\n" - "> > > +struct clk *clk_register_gate_shared(struct device *dev, const char =\n" - "*name,\n" + "> > > +struct clk *clk_register_gate_shared(struct device *dev, const char *name,\n" "> > > const char *parent_name, unsigned long flags,\n" "> > > void __iomem *reg, u8 bit_idx,\n" "> > > - u8 clk_gate_flags, spinlock_t *lock)\n" @@ -237,20 +191,16 @@ "> > > {\n" "> > > struct clk_gate *gate;\n" "> > > struct clk *clk;\n" - "> > > @@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev,=\n" - " const char *name,\n" - "> > > gate->bit_idx =3D bit_idx;\n" - "> > > gate->flags =3D clk_gate_flags;\n" - "> > > gate->lock =3D lock;\n" - "> > > + gate->share_count =3D share_count;\n" - "> > > gate->hw.init =3D &init;\n" - "> > > =\n" - "\n" - "> > > clk =3D clk_register(dev, &gate->hw);\n" - "> > > @@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev=\n" - ", const char *name,\n" - "> > > =\n" - "\n" + "> > > @@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,\n" + "> > > gate->bit_idx = bit_idx;\n" + "> > > gate->flags = clk_gate_flags;\n" + "> > > gate->lock = lock;\n" + "> > > + gate->share_count = share_count;\n" + "> > > gate->hw.init = &init;\n" + "> > > \n" + "> > > clk = clk_register(dev, &gate->hw);\n" + "> > > @@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev, const char *name,\n" + "> > > \n" "> > > return clk;\n" "> > > }\n" "> > > +\n" @@ -272,38 +222,30 @@ "> > > spinlock_t *lock;\n" "> > > + int *share_count;\n" "> > > };\n" - "> > > =\n" - "\n" + "> > > \n" "> > > #define CLK_GATE_SET_TO_DISABLE BIT(0)\n" - "> > > @@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev=\n" - ", const char *name,\n" + "> > > @@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev, const char *name,\n" "> > > const char *parent_name, unsigned long flags,\n" "> > > void __iomem *reg, u8 bit_idx,\n" "> > > u8 clk_gate_flags, spinlock_t *lock);\n" - "> > > +struct clk *clk_register_gate_shared(struct device *dev, const char =\n" - "*name,\n" + "> > > +struct clk *clk_register_gate_shared(struct device *dev, const char *name,\n" "> > > + const char *parent_name, unsigned long flags,\n" "> > > + void __iomem *reg, u8 bit_idx,\n" "> > > + u8 clk_gate_flags, spinlock_t *lock,\n" "> > > + int *share_count);\n" - "> > > =\n" - "\n" + "> > > \n" "> > > struct clk_div_table {\n" "> > > unsigned int val;\n" - "> > > =\n" - "\n" - "> > > =\n" - "\n" + "> > > \n" + "> > > \n" "> > > How to use these shared gates:\n" - "> > > =\n" - "\n" + "> > > \n" "> > > --- a/arch/powerpc/platforms/512x/clock-commonclk.c\n" "> > > +++ b/arch/powerpc/platforms/512x/clock-commonclk.c\n" "> > > @@ -123,6 +123,39 @@ static inline struct clk *mpc512x_clk_gated(\n" "> > > reg, pos, 0, &clklock);\n" "> > > }\n" - "> > > =\n" - "\n" + "> > > \n" "> > > +enum mpc512x_clk_shared_gate_id_t {\n" "> > > + MPC512x_CLK_SHARED_GATE_MSCAN,\n" "> > > + MPC512x_CLK_SHARED_GATE_MAX,\n" @@ -312,21 +254,15 @@ "> > > +static int mpc512x_clk_gate_counters[MPC512x_CLK_SHARED_GATE_MAX];\n" "> > > +\n" "> > > +/*\n" - "> > > + * implementor's note: since clk_gate items don't implement a separ=\n" - "ate\n" - "> > > + * .disable_unused() callback, their .disable() routine gets called =\n" - "and\n" - "> > > + * \"disable the clock as we can't see it's in use\" cannot be told fr=\n" - "om\n" + "> > > + * implementor's note: since clk_gate items don't implement a separate\n" + "> > > + * .disable_unused() callback, their .disable() routine gets called and\n" + "> > > + * \"disable the clock as we can't see it's in use\" cannot be told from\n" "> > > + * \"regular disable, count these events please\"\n" "> > > + *\n" - "> > > + * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppr=\n" - "ess\n" - "> > > + * the \"disable, unused\" call, so use counts won't get unbalanced, t=\n" - "he\n" + "> > > + * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppress\n" + "> > > + * the \"disable, unused\" call, so use counts won't get unbalanced, the\n" "> > > + * clock either never got enabled and thus need not get disabled, or\n" - "> > > + * part of the hardware got enabled while disabling the other part i=\n" - "sn't\n" + "> > > + * part of the hardware got enabled while disabling the other part isn't\n" "> > > + * wanted\n" "> > > + */\n" "> > > +static inline struct clk *mpc512x_clk_gated_shared(\n" @@ -336,49 +272,40 @@ "> > > +{\n" "> > > + int clkflags;\n" "> > > +\n" - "> > > + clkflags =3D CLK_SET_RATE_PARENT;\n" - "> > > + clkflags |=3D CLK_IGNORE_UNUSED;\n" - "> > > + return clk_register_gate_shared(NULL, name, parent_name, clkf=\n" - "lags,\n" + "> > > + clkflags = CLK_SET_RATE_PARENT;\n" + "> > > + clkflags |= CLK_IGNORE_UNUSED;\n" + "> > > + return clk_register_gate_shared(NULL, name, parent_name, clkflags,\n" "> > > + reg, pos, 0, &clklock,\n" - "> > > + &mpc512x_clk_gate_counters[sh=\n" - "are_id]);\n" + "> > > + &mpc512x_clk_gate_counters[share_id]);\n" "> > > +}\n" "> > > +\n" "> > > static inline struct clk *mpc512x_clk_muxed(const char *name,\n" "> > > const char **parent_names, int parent_count,\n" "> > > u32 __iomem *reg, u8 pos, u8 len)\n" - "> > > @@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_s=\n" - "etup_data *entry)\n" + "> > > @@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry)\n" "> > > 1, 1);\n" "> > > }\n" "> > > if (sccr_reg) {\n" - "> > > + if (entry->type =3D=3D MCLK_TYPE_MSCAN) {\n" - "> > > + clks[clks_idx_pub] =3D mpc512x_clk_gated_shar=\n" - "ed(\n" + "> > > + if (entry->type == MCLK_TYPE_MSCAN) {\n" + "> > > + clks[clks_idx_pub] = mpc512x_clk_gated_shared(\n" "> > > + entry->name_mclk,\n" - "> > > + entry->name_mux1, sccr_reg, s=\n" - "ccr_bit,\n" - "> > > + MPC512x_CLK_SHARED_GATE_MSCAN=\n" - ");\n" + "> > > + entry->name_mux1, sccr_reg, sccr_bit,\n" + "> > > + MPC512x_CLK_SHARED_GATE_MSCAN);\n" "> > > + } else {\n" - "> > > clks[clks_idx_pub] =3D mpc512x_clk_gated(\n" + "> > > clks[clks_idx_pub] = mpc512x_clk_gated(\n" "> > > entry->name_mclk,\n" - "> > > entry->name_mux1, sccr_reg, s=\n" - "ccr_bit);\n" + "> > > entry->name_mux1, sccr_reg, sccr_bit);\n" "> > > + }\n" "> > > } else {\n" - "> > > clks[clks_idx_pub] =3D mpc512x_clk_factor(\n" + "> > > clks[clks_idx_pub] = mpc512x_clk_factor(\n" "> > > entry->name_mclk,\n" - "> > > =\n" - "\n" + "> > > \n" "> > > Local tests have shown that the extension solves the problem of\n" "> > > how to satisfy the SoC's constraints on the MPC512x platform.\n" "> > > The MSCAN clocks no longer need to get pre-enabled, instead they\n" "> > > get setup and enabled only as the mscan(4) driver probes devices\n" "> > > according to how it was instructed (device tree nodes).\n" - "> > > =\n" - "\n" + "> > > \n" "> > > What do you think? Is the \"shared gate\" support in the common\n" "> > > logic appropriate? I'd rather not duplicate all of this code\n" "> > > just to introduce the specific gate I need, while most of the\n" @@ -386,16 +313,13 @@ "> > > desire isn't to override the gate's operations, but to wrap them\n" "> > > and to consult a counter in addition, while the register access\n" "> > > still applies.\n" - "> =\n" - "\n" - "> =\n" - "\n" + "> \n" + "> \n" "> virtually yours\n" "> Gerhard Sittig\n" - "> -- =\n" - "\n" + "> -- \n" "> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel\n" "> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany\n" - > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de + > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de -2d840ddefc21c26c02eca278a7c2d819f70ae9c57ad7b3cf08b17a9997b2829e +6cab6b537babb12eb8fd80fa564969871c353452cbdeace390b05ed21ae87405
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.