On czw, 2014-12-04 at 12:49 +0100, Sylwester Nawrocki wrote:
> On 04/12/14 11:47, Krzysztof Kozlowski wrote:
> > Audio subsystem clocks are located in separate block. If clock for this
> > block (from main clock domain) 'mau_epll' is gated then any read or
> > write to audss registers will block.
> >
> > This was observed on Exynos 5420 platforms (Arndale Octa and Peach
> > Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
> > commit the 'mau_epll' was gated, because the "amba" clock was disabled
> > and there were no more users of mau_epll. The system hang on disabling
> > unused clocks from audss block.
> >
> > Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.
> >
> > Whenever system wants to operate on audss clocks it has to enable epll
> > clock. The solution reuses common clk-gate/divider/mux code and duplicates
> > clk_register_*() functions.
> >
> > Additionally this patch fixes memory leak of clock gate/divider/mux
> > structures. The leak exists in generic clk_register_*() functions. Patch
> > replaces them with custom code with managed allocation.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlow...@samsung.com>
> > Reported-by: Javier Martinez Canillas <javier.marti...@collabora.co.uk>
> > Reported-by: Kevin Hilman <khil...@kernel.org>
> > ---
> > drivers/clk/samsung/clk-exynos-audss.c | 357
> > +++++++++++++++++++++++++++++----
> > 1 file changed, 321 insertions(+), 36 deletions(-)
>
> In general I'm not happy with this as we are more than doubling LOC in this
> driver. I don't have a better idea though ATM on how to address the issue for
> v3.19. I suspect we will need a regmap based clocks for some Exynos clocks
> controllers in near future and then we could refactor this driver by using
> the common code.
The regmap-mmio could solve it in a cleaner way but that would be much
bigger task.
>
> A few style comments below...
>
> > +/*
> > + * A simplified copy of clk-gate.c:clk_register_gate() to mimic
> > + * clk-gate behavior while using customized ops.
> > + */
> > +static struct clk *audss_clk_register_gate(struct device *dev, const char
> > *name,
> > + const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 bit_idx)
> > +{
> > + struct clk_gate *gate;
> > + struct clk *clk;
> > + struct clk_init_data init;
> > +
> > + /* allocate the gate */
> > + gate = devm_kzalloc(dev, sizeof(struct clk_gate), GFP_KERNEL);
> > + if (!gate)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = name;
> > + init.ops = &audss_clk_gate_ops;
> > + init.flags = flags | CLK_IS_BASIC;
> > + init.parent_names = (parent_name ? &parent_name : NULL);
> > + init.num_parents = (parent_name ? 1 : 0);
> > +
> > + /* struct clk_gate assignments */
> > + gate->reg = reg;
> > + gate->bit_idx = bit_idx;
> > + gate->flags = 0;
>
> The assignment here redundant, since the data structure has been allocated
> with kzalloc().
Sure. Most of these minor issues came from copying generic
clk_register_*().
>
> > + gate->lock = &lock;
> > + gate->hw.init = &init;
> > +
> > + clk = clk_register(dev, &gate->hw);
> > +
> > + return clk;
>
> Just do
> return clk_register(dev, &gate->hw);
OK.
>
> > +}
> > +
> > +static unsigned long audss_clk_divider_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + unsigned long ret;
> > +
> > + ret = pll_clk_enable();
> > + if (ret) {
> > + WARN(ret, "Could not enable epll clock\n");
> > + return parent_rate;
> > + }
>
> How about moving the error log to pll_clk_enable() and making this
>
> ret = pll_clk_enable();
> if (ret < 0)
> return parent_rate;
> ?
In other uses of pll_clk_enable(), the error is returned so I think
there is no need to print a warning. This is different because here
error cannot be returned.
> > +
> > + ret = clk_divider_ops.recalc_rate(hw, parent_rate);
> > +
> > + pll_clk_disable();
> > +
> > + return ret;
> > +}
> > +
>
> > +/*
> > + * A simplified copy of clk-divider.c:clk_register_divider() to mimic
> > + * clk-divider behavior while using customized ops.
> > + */
> > +static struct clk *audss_clk_register_divider(struct device *dev,
> > + const char *name, const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 shift, u8 width)
> > +{
> > + struct clk_divider *div;
> > + struct clk *clk;
> > + struct clk_init_data init;
> > +
> > + /* allocate the divider */
>
> Not sure if this comment helps in anything.
I'll remove it.
>
> > + div = devm_kzalloc(dev, sizeof(struct clk_divider), GFP_KERNEL);
> > + if (!div)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = name;
> > + init.ops = &audss_clk_divider_ops;
> > + init.flags = flags | CLK_IS_BASIC;
> > + init.parent_names = (parent_name ? &parent_name : NULL);
> > + init.num_parents = (parent_name ? 1 : 0);
> > +
> > + /* struct clk_divider assignments */
> > + div->reg = reg;
> > + div->shift = shift;
> > + div->width = width;
> > + div->flags = 0;
>
> Redundant assignment.
OK.
>
> > + div->lock = &lock;
> > + div->hw.init = &init;
> > + div->table = NULL;
>
> This could be safely removed as well, but up to you.
I'll remove it.
>
> > + /* register the clock */
>
> That comment has really no value, I'd remove it.
OK.
>
> > + clk = clk_register(dev, &div->hw);
> > +
> > + return clk;
>
> Please change it to:
>
> return clk_register(dev, &div->hw);
OK.
> > +}
> > +
> > +static u8 audss_clk_mux_get_parent(struct clk_hw *hw)
> > +{
> > + u8 parent;
> > + int ret;
> > +
> > + ret = pll_clk_enable();
> > + if (ret) {
> > + WARN(ret, "Could not enable epll clock\n");
> > + return -EINVAL; /* Just like clk_mux_get_parent() */
>
> Do we need to overwrite the error code ? The error log could be moved
> inside pll_clk_enable() and it all would become:
>
> ret = pll_clk_enable();
> if (ret < 0)
> return ret;
>
Similar to previous case - the warning is here because return value does
not accept error condition. I'll re-use existing error code but if you
don't mind I think warning should stay here.
> > + }
> > +
> > + parent = clk_mux_ops.get_parent(hw);
> > +
> > + pll_clk_disable();
> > +
> > + return parent;
> > +}
>
> > +/*
> > + * A simplified copy of clk-mux.c:clk_register_mux_table() to mimic
> > + * clk-mux behavior while using customized ops.
> > + */
> > +static struct clk *audss_clk_register_mux(struct device *dev, const char
> > *name,
> > + const char **parent_names, u8 num_parents, unsigned long flags,
> > + void __iomem *reg, u8 shift, u8 width)
> > +{
> > + struct clk_mux *mux;
> > + struct clk *clk;
> > + struct clk_init_data init;
> > + u32 mask = BIT(width) - 1;
> > +
> > + /* allocate the mux */
>
> I'd remove this comment.
OK.
>
> > + mux = devm_kzalloc(dev, sizeof(struct clk_mux), GFP_KERNEL);
> > + if (!mux)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = name;
> > + init.ops = &audss_clk_mux_ops;
> > + init.flags = flags | CLK_IS_BASIC;
> > + init.parent_names = parent_names;
> > + init.num_parents = num_parents;
> > +
> > + /* struct clk_mux assignments */
> > + mux->reg = reg;
> > + mux->shift = shift;
> > + mux->mask = mask;
> > + mux->flags = 0;
>
> Redundant assignment.
OK.
>
> > + mux->lock = &lock;
> > + mux->table = NULL;
>
> Ditto.
OK.
>
> > + mux->hw.init = &init;
> > +
> > + clk = clk_register(dev, &mux->hw);
> > +
> > + return clk;
>
> return clk_register(dev, &mux->hw);
OK.
> > +}
> > +
> > /* register exynos_audss clocks */
> > static int exynos_audss_clk_probe(struct platform_device *pdev)
> > {
> > @@ -98,6 +364,8 @@ static int exynos_audss_clk_probe(struct platform_device
> > *pdev)
> > dev_err(&pdev->dev, "failed to map audss registers\n");
> > return PTR_ERR(reg_base);
> > }
> > + /* epll don't have to be enabled for boards != Exynos5420 */
>
> s/!=/other than/ ?
> s/epll/EPLL ?
OK.
Thanks for feedback!
Best regards,
Krzysztof
>
> > + epll = ERR_PTR(-ENODEV);
> >
> > clk_table = devm_kzalloc(&pdev->dev,
> > sizeof(struct clk *) * EXYNOS_AUDSS_MAX_CLKS,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
