From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932270AbcLLXja (ORCPT ); Mon, 12 Dec 2016 18:39:30 -0500 Received: from mail-oi0-f65.google.com ([209.85.218.65]:35528 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751465AbcLLXj1 (ORCPT ); Mon, 12 Dec 2016 18:39:27 -0500 MIME-Version: 1.0 In-Reply-To: <1481306510-7471-2-git-send-email-irina.tirdea@intel.com> References: <1481306510-7471-1-git-send-email-irina.tirdea@intel.com> <1481306510-7471-2-git-send-email-irina.tirdea@intel.com> From: Andy Shevchenko Date: Tue, 13 Dec 2016 01:39:24 +0200 Message-ID: Subject: Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks To: Irina Tirdea Cc: linux-clk@vger.kernel.org, "x86@kernel.org" , platform-driver-x86@vger.kernel.org, Stephen Boyd , Darren Hart , Thomas Gleixner , Michael Turquette , Ingo Molnar , "H. Peter Anvin" , ALSA Development Mailing List , Mark Brown , Takashi Iwai , Pierre-Louis Bossart , "Rafael J. Wysocki" , Len Brown , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Pierre-Louis Bossart Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea wrote: Thanks for an update I will comment all the patches. Here we start. > The BayTrail and CherryTrail platforms provide platform clocks > through their Power Management Controller (PMC). > > The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a > frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail > and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks > are available for general system use, where appropriate, and each > have Control & Frequency register fields associated with them. > > Signed-off-by: Irina Tirdea > Signed-off-by: Pierre-Louis Bossart Who is the actual author? SoB I guess should be either the author, or 1st, 2nd, ..., last one who is submitter. > --- a/drivers/clk/x86/Makefile > +++ b/drivers/clk/x86/Makefile > @@ -1,2 +1,5 @@ > clk-x86-lpss-objs := clk-lpt.o > obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o > +ifeq ($(CONFIG_COMMON_CLK), y) Hmm... I think (I didn't check) we don't go here otherwise. > +obj-$(CONFIG_PMC_ATOM) += clk-byt-plt.o I'm pretty sure X86_INTEL_LPSS almost replicates what you need (it also includes Haswell support, but it doesn't matter here). Can we unify them or is it a bad idea? Otherwise I would propose to rename module to be something like clk-x86-pmc.o which follows above pattern: LPSS as provider, PMC as provider and so on. Maybe clk-x86-pmc-objs := clk-pmc-atom.o ... By the way lpt is a not good chosen abbreviation for Lynxpoint. I even had a patch to get rid of this file completely. > +endif > diff --git a/drivers/clk/x86/clk-byt-plt.c b/drivers/clk/x86/clk-byt-plt.c > new file mode 100644 > index 0000000..2303e0d > --- /dev/null > +++ b/drivers/clk/x86/clk-byt-plt.c > @@ -0,0 +1,380 @@ > +/* > + * Intel Atom platform clocks driver for BayTrail and CherryTrail SoC. SoCs. > + * > + * Copyright (C) 2016, Intel Corporation > + * Author: Irina Tirdea Be consistent with SoB lines above. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include Is it indeed platform data? I would not create platform_data/x86 without strong argument. Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h which is old enough and was basically first try of clk stuff on x86) > + > +#define PLT_CLK_NAME_BASE "pmc_plt_clk_" > +#define PLT_CLK_DRIVER_NAME "clk-byt-plt" By default you may use build name of the module, but if you want to stick with something choose the name I mentioned above like clk-pmc-atom. > > +#define PMC_CLK_CTL_SIZE 4 > +#define PMC_CLK_NUM 6 > +#define PMC_MASK_CLK_CTL GENMASK(1, 0) > +#define PMC_MASK_CLK_FREQ BIT(2) > +#define PMC_CLK_CTL_GATED_ON_D3 0x0 > +#define PMC_CLK_CTL_FORCE_ON 0x1 > +#define PMC_CLK_CTL_FORCE_OFF 0x2 > +#define PMC_CLK_CTL_RESERVED 0x3 > +#define PMC_CLK_FREQ_XTAL 0x0 /* 25 MHz */ > +#define PMC_CLK_FREQ_PLL 0x4 /* 19.2 MHz */ Looks like (0 << 2) and (1 << 2). I would put that way to show that it's bitwise value. > + > +struct clk_plt_fixed { Yeah, rename names accordingly. > + struct clk_hw *clk; > + struct clk_lookup *lookup; > +}; > + > +struct clk_plt { > + struct clk_hw hw; > + void __iomem *reg; > + struct clk_lookup *lookup; > + spinlock_t lock; Would be nice to have a comment what is/are protected by it. > +}; > + > +#define to_clk_plt(_hw) container_of(_hw, struct clk_plt, hw) > + > +struct clk_plt_data { > + struct clk_plt_fixed **parents; > + u8 nparents; > + struct clk_plt *clks[PMC_CLK_NUM]; > +}; > + > +static inline int plt_reg_to_parent(int reg) > +{ > + switch (reg & PMC_MASK_CLK_FREQ) { + default: (see below) ? > + case PMC_CLK_FREQ_XTAL: > + return 0; /* index 0 in parents[] */ > + case PMC_CLK_FREQ_PLL: > + return 1; /* index 1 in parents[] */ > + } > + > + return 0; default: ? > +} > + > +static inline int plt_parent_to_reg(int index) > +{ > + switch (index) { > + case 0: /* index 0 in parents[] */ > + return PMC_CLK_FREQ_XTAL; > + case 1: /* index 0 in parents[] */ > + return PMC_CLK_FREQ_PLL; > + } > + > + return PMC_CLK_FREQ_XTAL; Ditto. > +} > + > +static inline int plt_reg_to_enabled(int reg) > +{ > + switch (reg & PMC_MASK_CLK_CTL) { > + case PMC_CLK_CTL_GATED_ON_D3: > + case PMC_CLK_CTL_FORCE_ON: > + return 1; /* enabled */ > + case PMC_CLK_CTL_FORCE_OFF: > + case PMC_CLK_CTL_RESERVED: > + default: > + return 0; /* disabled */ > + } > +} > + > +static void plt_clk_reg_update(struct clk_plt *clk, u32 mask, u32 val) > +{ > + u32 orig, tmp; > + unsigned long flags = 0; Redundant assignment. > + > + spin_lock_irqsave(&clk->lock, flags); > + > + orig = readl(clk->reg); > + > + tmp = orig & ~mask; > + tmp |= val & mask; > + > + if (tmp != orig) Hmm...Is here any benefit? Do we do this 1000s times per ...s? OTOH readability a bit better w/o it. > + writel(tmp, clk->reg); > + > + spin_unlock_irqrestore(&clk->lock, flags); > +} > + > +static int plt_clk_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct clk_plt *clk = to_clk_plt(hw); > + > + plt_clk_reg_update(clk, PMC_MASK_CLK_FREQ, plt_parent_to_reg(index)); > + > + return 0; > +} > + > +static u8 plt_clk_get_parent(struct clk_hw *hw) > +{ > + struct clk_plt *clk = to_clk_plt(hw); > + u32 value; > + > + value = readl(clk->reg); > + > + return plt_reg_to_parent(value); > +} > + > +static int plt_clk_enable(struct clk_hw *hw) > +{ > + struct clk_plt *clk = to_clk_plt(hw); > + > + plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_ON); > + > + return 0; > +} > + > +static void plt_clk_disable(struct clk_hw *hw) > +{ > + struct clk_plt *clk = to_clk_plt(hw); > + > + plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_OFF); > +} > + > +static int plt_clk_is_enabled(struct clk_hw *hw) > +{ > + struct clk_plt *clk = to_clk_plt(hw); > + u32 value; > + > + value = readl(clk->reg); > + > + return plt_reg_to_enabled(value); > +} > + > +static const struct clk_ops plt_clk_ops = { > + .enable = plt_clk_enable, > + .disable = plt_clk_disable, > + .is_enabled = plt_clk_is_enabled, > + .get_parent = plt_clk_get_parent, > + .set_parent = plt_clk_set_parent, > + .determine_rate = __clk_mux_determine_rate, > +}; > + > +static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, I don't see how pdev is involved, perhaps just struct device *dev here. > + void __iomem *base, > + const char **parent_names, > + int num_parents) > +{ > + struct clk_plt *pclk; > + struct clk_init_data init; > + int ret; > + > + pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL); > + if (!pclk) > + return ERR_PTR(-ENOMEM); > + > + init.name = kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id); devm_kasprintf() > + init.ops = &plt_clk_ops; > + init.flags = 0; > + init.parent_names = parent_names; > + init.num_parents = num_parents; > + > + pclk->hw.init = &init; > + pclk->reg = base + id * PMC_CLK_CTL_SIZE; > + spin_lock_init(&pclk->lock); > + > + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > + if (ret) > + goto err_free_init; > + > + pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL); > + if (!pclk->lookup) { > + ret = -ENOMEM; > + goto err_free_init; > + } > + > + kfree(init.name); devm_kfree(); > + > + return pclk; > + > +err_free_init: > + kfree(init.name); > + return ERR_PTR(ret); Might be redundant, see above. > +} > + > +static void plt_clk_unregister(struct clk_plt *pclk) > +{ > + clkdev_drop(pclk->lookup); > +} > + > +static struct clk_plt_fixed *plt_clk_register_fixed_rate(struct platform_device *pdev, > + const char *name, > + const char *parent_name, > + unsigned long fixed_rate) > +{ > + struct clk_plt_fixed *pclk; > + int ret = 0; Useless assignment. > + > + pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL); > + if (!pclk) > + return ERR_PTR(-ENOMEM); > + > + pclk->clk = clk_hw_register_fixed_rate(&pdev->dev, name, parent_name, > + 0, fixed_rate); > + if (IS_ERR(pclk->clk)) > + return ERR_CAST(pclk->clk); > + > + pclk->lookup = clkdev_hw_create(pclk->clk, name, NULL); > + if (!pclk->lookup) { > + ret = -ENOMEM; > + goto err_clk_unregister; > + } > + > + return pclk; > + > +err_clk_unregister: > + clk_hw_unregister_fixed_rate(pclk->clk); > + return ERR_PTR(ret); > +} > + > +static void plt_clk_unregister_fixed_rate(struct clk_plt_fixed *pclk) > +{ > + clkdev_drop(pclk->lookup); > + clk_hw_unregister_fixed_rate(pclk->clk); > +} > + > +static const char **plt_clk_register_parents(struct platform_device *pdev, > + struct clk_plt_data *data, > + const struct pmc_clk *clks) > +{ > + const char **parent_names; > + int i, err; > + > + data->nparents = 0; > + while (clks[data->nparents].name) > + data->nparents++; > + > + data->parents = devm_kcalloc(&pdev->dev, data->nparents, > + sizeof(*data->parents), GFP_KERNEL); > + if (!data->parents) { > + err = -ENOMEM; > + goto err_out; > + } > + > + parent_names = kcalloc(data->nparents, sizeof(*parent_names), > + GFP_KERNEL); > + if (!parent_names) { > + err = -ENOMEM; > + goto err_out; > + } > + > + for (i = 0; i < data->nparents; i++) { > + data->parents[i] = > + plt_clk_register_fixed_rate(pdev, clks[i].name, > + clks[i].parent_name, > + clks[i].freq); > + if (IS_ERR(data->parents[i])) { > + err = PTR_ERR(data->parents[i]); > + goto err_unreg; > + } > + parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL); > + } > + > + return parent_names; > + > +err_unreg: > + for (i--; i >= 0; i--) { while (i--) { } > + plt_clk_unregister_fixed_rate(data->parents[i]); > + kfree_const(parent_names[i]); > + } > + kfree(parent_names); > +err_out: > + data->nparents = 0; You will not need this if you define local variable for nparents and assign data->nparents at last in the function. > + return ERR_PTR(err); > +} > + > +static void plt_clk_unregister_parents(struct clk_plt_data *data) > +{ > + int i; > + > + for (i = 0; i < data->nparents; i++) > + plt_clk_unregister_fixed_rate(data->parents[i]); > +} > + > +static int plt_clk_probe(struct platform_device *pdev) > +{ > + struct clk_plt_data *data; > + int i, err; > + const char **parent_names; > + const struct pmc_clk_data *pmc_data; Reversed order, please. Usually: assignments at very beginning, longer first, short later, error code variable last, flags for spin lock -- depends. > + > + pmc_data = dev_get_platdata(&pdev->dev); > + if (!pmc_data || !pmc_data->clks) > + return -EINVAL; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + parent_names = plt_clk_register_parents(pdev, data, pmc_data->clks); > + if (IS_ERR(parent_names)) > + return PTR_ERR(parent_names); > + > + for (i = 0; i < PMC_CLK_NUM; i++) { > + data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, > + parent_names, data->nparents); > + if (IS_ERR(data->clks[i])) { > + err = PTR_ERR(data->clks[i]); > + goto err_unreg_clk_plt; > + } > + } > + > + for (i = 0; i < data->nparents; i++) > + kfree_const(parent_names[i]); > + kfree(parent_names); (1) > + > + dev_set_drvdata(&pdev->dev, data); > + return 0; > + > +err_unreg_clk_plt: > + for (i--; i >= 0; i--) > + plt_clk_unregister(data->clks[i]); > + plt_clk_unregister_parents(data); (3) > + for (i = 0; i < data->nparents; i++) > + kfree_const(parent_names[i]); > + kfree(parent_names); (2) (1) and (2) -> helper function? > + return err; > +} > + > +static int plt_clk_remove(struct platform_device *pdev) > +{ > + struct clk_plt_data *data; > + int i; > + > + data = dev_get_drvdata(&pdev->dev); > + if (!data) > + return 0; > + > + for (i = 0; i < PMC_CLK_NUM; i++) > + plt_clk_unregister(data->clks[i]); > + plt_clk_unregister_parents(data); (4) (3) and (4) -> helper function ? > + return 0; > +} > + > +static struct platform_driver plt_clk_driver = { > + .driver = { > + .name = PLT_CLK_DRIVER_NAME, Better to put such inplace here. You know why? Instead of one git grep one has to run two in order to find actual driver name. > + }, > + .probe = plt_clk_probe, > + .remove = plt_clk_remove, > +}; > +module_platform_driver(plt_clk_driver); > + > +MODULE_DESCRIPTION("Intel Atom platform clocks driver"); > +MODULE_AUTHOR("Irina Tirdea "); Be consistent with SoB lines > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/platform_data/x86/clk-byt-plt.h b/include/linux/platform_data/x86/clk-byt-plt.h > new file mode 100644 > index 0000000..e6bca9c > --- /dev/null > +++ b/include/linux/platform_data/x86/clk-byt-plt.h > @@ -0,0 +1,31 @@ > +/* > + * Intel Atom platform clocks for BayTrail and CherryTrail SoC. > + * > + * Copyright (C) 2016, Intel Corporation > + * Author: Irina Tirdea Ditto. Of course in all cases exceptions are possible (if another author has done partial stuff) > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +#ifndef __CLK_BYT_PLT_H > +#define __CLK_BYT_PLT_H > + > +struct pmc_clk { > + const char *name; > + unsigned long freq; > + const char *parent_name; > +}; > + > +struct pmc_clk_data { > + void __iomem *base; > + const struct pmc_clk *clks; > +}; Those are definitely do not look like a *platform data* at all. > + > +#endif /* __CLK_BYT_PLT_H */ -- With Best Regards, Andy Shevchenko