Message ID | 20210317193006.29633-3-digetx@gmail.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | Couple improvements for Tegra clk driver | expand |
On Wed, Mar 17, 2021 at 10:30:01PM +0300, Dmitry Osipenko wrote: > The refcounting of the gate clocks has a bug causing the enable_refcnt > to underflow when unused clocks are disabled. This happens because clk > provider erroneously bumps the refcount if clock is enabled at a boot > time, which it shouldn't be doing, and it does this only for the gate > clocks, while peripheral clocks are using the same gate ops and the > peripheral clocks are missing the initial bump. Hence the refcount of > the peripheral clocks is 0 when unused clocks are disabled and then the > counter is decremented further by the gate ops, causing the integer > underflow. [...] > diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c > index 4b31beefc9fc..3c4259fec82e 100644 > --- a/drivers/clk/tegra/clk-periph-gate.c > +++ b/drivers/clk/tegra/clk-periph-gate.c [...] > @@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw) > > spin_lock_irqsave(&periph_ref_lock, flags); > > - gate->enable_refcnt[gate->clk_num]--; > - if (gate->enable_refcnt[gate->clk_num] > 0) { > - spin_unlock_irqrestore(&periph_ref_lock, flags); > - return; > - } > + WARN_ON(!gate->enable_refcnt[gate->clk_num]); > + > + if (gate->enable_refcnt[gate->clk_num]-- == 1) > + clk_periph_disable_locked(hw); Nit: "if (--n == 0)" seems more natural, as you want to call clk_periph_disable_locked() when the refcount goes down to 0. [...] > /* > - * If peripheral is in the APB bus then read the APB bus to > - * flush the write operation in apb bus. This will avoid the > - * peripheral access after disabling clock > + * Some clocks are duplicated and some of them are marked as critical, > + * like fuse and fuse_burn for example, thus the enable_refcnt will > + * be non-zero here id the "unused" duplicate is disabled by CCF. s/id/if/ ? Best Regards Michał Mirosław
18.03.2021 12:12, Michał Mirosław пишет: > On Wed, Mar 17, 2021 at 10:30:01PM +0300, Dmitry Osipenko wrote: >> The refcounting of the gate clocks has a bug causing the enable_refcnt >> to underflow when unused clocks are disabled. This happens because clk >> provider erroneously bumps the refcount if clock is enabled at a boot >> time, which it shouldn't be doing, and it does this only for the gate >> clocks, while peripheral clocks are using the same gate ops and the >> peripheral clocks are missing the initial bump. Hence the refcount of >> the peripheral clocks is 0 when unused clocks are disabled and then the >> counter is decremented further by the gate ops, causing the integer >> underflow. > [...] >> diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c >> index 4b31beefc9fc..3c4259fec82e 100644 >> --- a/drivers/clk/tegra/clk-periph-gate.c >> +++ b/drivers/clk/tegra/clk-periph-gate.c > [...] >> @@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw) >> >> spin_lock_irqsave(&periph_ref_lock, flags); >> >> - gate->enable_refcnt[gate->clk_num]--; >> - if (gate->enable_refcnt[gate->clk_num] > 0) { >> - spin_unlock_irqrestore(&periph_ref_lock, flags); >> - return; >> - } >> + WARN_ON(!gate->enable_refcnt[gate->clk_num]); >> + >> + if (gate->enable_refcnt[gate->clk_num]-- == 1) >> + clk_periph_disable_locked(hw); > > Nit: "if (--n == 0)" seems more natural, as you want to call > clk_periph_disable_locked() when the refcount goes down to 0. > > [...] >> /* >> - * If peripheral is in the APB bus then read the APB bus to >> - * flush the write operation in apb bus. This will avoid the >> - * peripheral access after disabling clock >> + * Some clocks are duplicated and some of them are marked as critical, >> + * like fuse and fuse_burn for example, thus the enable_refcnt will >> + * be non-zero here id the "unused" duplicate is disabled by CCF. > > s/id/if/ ? I'll update this patch over the weekend, thanks!
diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c index 4b31beefc9fc..3c4259fec82e 100644 --- a/drivers/clk/tegra/clk-periph-gate.c +++ b/drivers/clk/tegra/clk-periph-gate.c @@ -48,18 +48,9 @@ static int clk_periph_is_enabled(struct clk_hw *hw) return state; } -static int clk_periph_enable(struct clk_hw *hw) +static void clk_periph_enable_locked(struct clk_hw *hw) { struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); - unsigned long flags = 0; - - spin_lock_irqsave(&periph_ref_lock, flags); - - gate->enable_refcnt[gate->clk_num]++; - if (gate->enable_refcnt[gate->clk_num] > 1) { - spin_unlock_irqrestore(&periph_ref_lock, flags); - return 0; - } write_enb_set(periph_clk_to_bit(gate), gate); udelay(2); @@ -78,6 +69,32 @@ static int clk_periph_enable(struct clk_hw *hw) udelay(1); writel_relaxed(0, gate->clk_base + LVL2_CLK_GATE_OVRE); } +} + +static void clk_periph_disable_locked(struct clk_hw *hw) +{ + struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); + + /* + * If peripheral is in the APB bus then read the APB bus to + * flush the write operation in apb bus. This will avoid the + * peripheral access after disabling clock + */ + if (gate->flags & TEGRA_PERIPH_ON_APB) + tegra_read_chipid(); + + write_enb_clr(periph_clk_to_bit(gate), gate); +} + +static int clk_periph_enable(struct clk_hw *hw) +{ + struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); + unsigned long flags = 0; + + spin_lock_irqsave(&periph_ref_lock, flags); + + if (!gate->enable_refcnt[gate->clk_num]++) + clk_periph_enable_locked(hw); spin_unlock_irqrestore(&periph_ref_lock, flags); @@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw) spin_lock_irqsave(&periph_ref_lock, flags); - gate->enable_refcnt[gate->clk_num]--; - if (gate->enable_refcnt[gate->clk_num] > 0) { - spin_unlock_irqrestore(&periph_ref_lock, flags); - return; - } + WARN_ON(!gate->enable_refcnt[gate->clk_num]); + + if (gate->enable_refcnt[gate->clk_num]-- == 1) + clk_periph_disable_locked(hw); + + spin_unlock_irqrestore(&periph_ref_lock, flags); +} + +static void clk_periph_disable_unused(struct clk_hw *hw) +{ + struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); + unsigned long flags = 0; + + spin_lock_irqsave(&periph_ref_lock, flags); /* - * If peripheral is in the APB bus then read the APB bus to - * flush the write operation in apb bus. This will avoid the - * peripheral access after disabling clock + * Some clocks are duplicated and some of them are marked as critical, + * like fuse and fuse_burn for example, thus the enable_refcnt will + * be non-zero here id the "unused" duplicate is disabled by CCF. */ - if (gate->flags & TEGRA_PERIPH_ON_APB) - tegra_read_chipid(); - - write_enb_clr(periph_clk_to_bit(gate), gate); + if (!gate->enable_refcnt[gate->clk_num]) + clk_periph_disable_locked(hw); spin_unlock_irqrestore(&periph_ref_lock, flags); } @@ -114,6 +138,7 @@ const struct clk_ops tegra_clk_periph_gate_ops = { .is_enabled = clk_periph_is_enabled, .enable = clk_periph_enable, .disable = clk_periph_disable, + .disable_unused = clk_periph_disable_unused, }; struct clk *tegra_clk_register_periph_gate(const char *name, @@ -148,9 +173,6 @@ struct clk *tegra_clk_register_periph_gate(const char *name, gate->enable_refcnt = enable_refcnt; gate->regs = pregs; - if (read_enb(gate) & periph_clk_to_bit(gate)) - enable_refcnt[clk_num]++; - /* Data in .init is copied by clk_register(), so stack variable OK */ gate->hw.init = &init; diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c index 67620c7ecd9e..79ca3aa072b7 100644 --- a/drivers/clk/tegra/clk-periph.c +++ b/drivers/clk/tegra/clk-periph.c @@ -100,6 +100,15 @@ static void clk_periph_disable(struct clk_hw *hw) gate_ops->disable(gate_hw); } +static void clk_periph_disable_unused(struct clk_hw *hw) +{ + struct tegra_clk_periph *periph = to_clk_periph(hw); + const struct clk_ops *gate_ops = periph->gate_ops; + struct clk_hw *gate_hw = &periph->gate.hw; + + gate_ops->disable_unused(gate_hw); +} + static void clk_periph_restore_context(struct clk_hw *hw) { struct tegra_clk_periph *periph = to_clk_periph(hw); @@ -126,6 +135,7 @@ const struct clk_ops tegra_clk_periph_ops = { .is_enabled = clk_periph_is_enabled, .enable = clk_periph_enable, .disable = clk_periph_disable, + .disable_unused = clk_periph_disable_unused, .restore_context = clk_periph_restore_context, }; @@ -135,6 +145,7 @@ static const struct clk_ops tegra_clk_periph_nodiv_ops = { .is_enabled = clk_periph_is_enabled, .enable = clk_periph_enable, .disable = clk_periph_disable, + .disable_unused = clk_periph_disable_unused, .restore_context = clk_periph_restore_context, };