| Message ID | 1459520791-13269-2-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/01, Neil Armstrong wrote: > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 16f7d33..2efdbab 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -197,6 +197,12 @@ config COMMON_CLK_PXA > ---help--- > Support for the Marvell PXA SoC. > > +config COMMON_CLK_OXNAS > + bool Please add some text in quotes here so that this is user visible. > + select MFD_SYSCON > + ---help--- > + Support for the OXNAS SoC Family clocks. > + > diff --git a/drivers/clk/clk-oxnas.c b/drivers/clk/clk-oxnas.c > new file mode 100644 > index 0000000..5f02cfa > --- /dev/null > +++ b/drivers/clk/clk-oxnas.c > @@ -0,0 +1,202 @@ > + > +/* Clk init data declaration */ > +DECLARE_STD_CLK(leon); > +DECLARE_STD_CLK(dma_sgdma); > +DECLARE_STD_CLK(cipher); > +DECLARE_STD_CLK(sata); > +DECLARE_STD_CLK(audio); > +DECLARE_STD_CLK(usbmph); > +DECLARE_STD_CLKP(etha, eth_parents); > +DECLARE_STD_CLK(pciea); > +DECLARE_STD_CLK(nand); > + > +/* Bit - Name association */ > +static const struct clk_init_data *clk_oxnas_init[] = { > + [0] = &clk_leon_init, > + [1] = &clk_dma_sgdma_init, > + [2] = &clk_cipher_init, > + [3] = NULL, /* Do not touch to DDR clock */ Why do we have this here then? The dt binding has different numbers, so having this array with a hole in it seems fragile when we're using the order of this array to map to the clk indices. > + [4] = &clk_sata_init, > + [5] = &clk_audio_init, > + [6] = &clk_usbmph_init, > + [7] = &clk_etha_init, > + [8] = &clk_pciea_init, > + [9] = &clk_nand_init, > +}; > + > +static int oxnas_stdclk_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct regmap *regmap; > + struct clk_oxnas *clk_oxnas; > + struct clk_onecell_data *onecell_data; > + struct clk **clks; > + unsigned int clks_count = 0; > + int i; > + > + clk_oxnas = devm_kzalloc(&pdev->dev, > + sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init), > + GFP_KERNEL); devm_kcalloc()? > + if (!clk_oxnas) > + return -ENOMEM; > + > + clks = devm_kzalloc(&pdev->dev, > + sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init), > + GFP_KERNEL); devm_kcalloc()? This can be one smaller because DDR is never touched? > + if (!clks) > + return -ENOMEM; > + > + onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data), > + GFP_KERNEL); Maybe we should have one structure that we allocate all at once? > + if (!onecell_data) > + return -ENOMEM; > + > + regmap = syscon_node_to_regmap(of_get_parent(np)); Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd prefer device APIs over DT APIs here. > + if (!regmap) { > + dev_err(&pdev->dev, "failed to have parent regmap\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) { > + struct clk_oxnas *_clk; > + > + if (!clk_oxnas_init[i]) > + continue; > + > + _clk = &clk_oxnas[i]; > + _clk->bit = i; Ah I see now, the order is used to encode bit offset. The comment above the init data array could be expanded a bit then please. > + _clk->hw.init = clk_oxnas_init[i]; > + _clk->regmap = regmap; > + > + clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw); > + if (WARN_ON(IS_ERR(clks[clks_count]))) > + return PTR_ERR(clks[clks_count]); > + > + ++clks_count; > + } > + > + onecell_data->clks = clks; > + onecell_data->clk_num = clks_count; > + > + return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data); > +} > + > +static const struct of_device_id oxnas_stdclk_dt_ids[] = { > + { .compatible = "oxsemi,ox810se-stdclk" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids); > + > +static struct platform_driver oxnas_stdclk_driver = { > + .probe = oxnas_stdclk_probe, > + .remove = oxnas_stdclk_remove, > + .driver = { > + .name = "oxnas-stdclk", > + .of_match_table = of_match_ptr(oxnas_stdclk_dt_ids), You can drop of_match_ptr() here, it will just lead to unused variable warnings.
On 04/02/2016 02:50 AM, Stephen Boyd wrote: >> + >> +/* Bit - Name association */ >> +static const struct clk_init_data *clk_oxnas_init[] = { >> + [0] = &clk_leon_init, >> + [1] = &clk_dma_sgdma_init, >> + [2] = &clk_cipher_init, >> + [3] = NULL, /* Do not touch to DDR clock */ > > Why do we have this here then? The dt binding has different > numbers, so having this array with a hole in it seems fragile > when we're using the order of this array to map to the clk > indices. Hi Stephen, I skipped the DDR clock since it must be touched in any case, I could have introduced a read-only clock.... I already posted an arm-soc dts pull request with the DTSI using the bindings indices. Here the array indice is the register bit index, maybe I can order by bindings indice and add the hardware bit in an intermediate structure ? >> + clk_oxnas = devm_kzalloc(&pdev->dev, >> + sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init), >> + GFP_KERNEL); > > devm_kcalloc()? > >> + if (!clk_oxnas) >> + return -ENOMEM; >> + >> + clks = devm_kzalloc(&pdev->dev, >> + sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init), >> + GFP_KERNEL); > > devm_kcalloc()? This can be one smaller because DDR is never > touched? Sure > >> + if (!clks) >> + return -ENOMEM; >> + >> + onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data), >> + GFP_KERNEL); > > Maybe we should have one structure that we allocate all at once? > >> + if (!onecell_data) >> + return -ENOMEM; >> + >> + regmap = syscon_node_to_regmap(of_get_parent(np)); > > Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd > prefer device APIs over DT APIs here. Ok >> + if (!regmap) { >> + dev_err(&pdev->dev, "failed to have parent regmap\n"); >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) { >> + struct clk_oxnas *_clk; >> + >> + if (!clk_oxnas_init[i]) >> + continue; >> + >> + _clk = &clk_oxnas[i]; >> + _clk->bit = i; > > Ah I see now, the order is used to encode bit offset. The comment > above the init data array could be expanded a bit then please. Sure it need clarifications ! > >> + _clk->hw.init = clk_oxnas_init[i]; >> + _clk->regmap = regmap; >> + >> + clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw); >> + if (WARN_ON(IS_ERR(clks[clks_count]))) >> + return PTR_ERR(clks[clks_count]); >> + >> + ++clks_count; >> + } >> + >> + onecell_data->clks = clks; >> + onecell_data->clk_num = clks_count; >> + >> + return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data); >> +} >> + >> +static const struct of_device_id oxnas_stdclk_dt_ids[] = { >> + { .compatible = "oxsemi,ox810se-stdclk" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids); >> + >> +static struct platform_driver oxnas_stdclk_driver = { >> + .probe = oxnas_stdclk_probe, >> + .remove = oxnas_stdclk_remove, >> + .driver = { >> + .name = "oxnas-stdclk", >> + .of_match_table = of_match_ptr(oxnas_stdclk_dt_ids), > > You can drop of_match_ptr() here, it will just lead to unused > variable warnings. OK, thanks for the review, I hope the next post will go throught 4.7 ! Regards, Neil
On 04/02/2016 02:50 AM, Stephen Boyd wrote: > On 04/01, Neil Armstrong wrote: >> + if (!onecell_data) >> + return -ENOMEM; >> + >> + regmap = syscon_node_to_regmap(of_get_parent(np)); > > Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd > prefer device APIs over DT APIs here. > It will not work here since the parent node is a syscon, the call to syscon_node_to_regmap() will call of_syscon_register() and create the regmap, the dev_get_regmap() needs a proper platform device registered as regmap here. Neil
On 04/03, Neil Armstrong wrote: > On 04/02/2016 02:50 AM, Stephen Boyd wrote: > > On 04/01, Neil Armstrong wrote: > >> + if (!onecell_data) > >> + return -ENOMEM; > >> + > >> + regmap = syscon_node_to_regmap(of_get_parent(np)); > > > > Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd > > prefer device APIs over DT APIs here. > > > > It will not work here since the parent node is a syscon, the call to syscon_node_to_regmap() will call of_syscon_register() and create the regmap, the dev_get_regmap() needs a proper platform device registered as regmap here. > Ok. I was hoping that we could make simple-mfd look to see if there's a syscon and then attach it to the parent device, but it seems that simple-mfd is not actually a driver and it might not even make a parent device for the children nodes?
On 04/08/2016 01:31 AM, Stephen Boyd wrote: > On 04/03, Neil Armstrong wrote: >> On 04/02/2016 02:50 AM, Stephen Boyd wrote: > > Ok. I was hoping that we could make simple-mfd look to see if > there's a syscon and then attach it to the parent device, but it > seems that simple-mfd is not actually a driver and it might not > even make a parent device for the children nodes? > It seems so, I will look at the device hierarchy next week. Syscon is only used by these syscon_node_to_regmap() or similar calls, and they are the only APIs available. This call is at least used by clk-mtk, nxp and at91 clocks, is there a plan for these to change to another API ? Neil
On 04/11, Neil Armstrong wrote: > On 04/08/2016 01:31 AM, Stephen Boyd wrote: > > On 04/03, Neil Armstrong wrote: > >> On 04/02/2016 02:50 AM, Stephen Boyd wrote: > > > > Ok. I was hoping that we could make simple-mfd look to see if > > there's a syscon and then attach it to the parent device, but it > > seems that simple-mfd is not actually a driver and it might not > > even make a parent device for the children nodes? > > > > It seems so, I will look at the device hierarchy next week. > Syscon is only used by these syscon_node_to_regmap() or similar calls, and they are the only APIs available. > > This call is at least used by clk-mtk, nxp and at91 clocks, is there a plan for these to change to another API ? No plans. I'm just complaining.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 16f7d33..2efdbab 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -197,6 +197,12 @@ config COMMON_CLK_PXA ---help--- Support for the Marvell PXA SoC. +config COMMON_CLK_OXNAS + bool + select MFD_SYSCON + ---help--- + Support for the OXNAS SoC Family clocks. + source "drivers/clk/bcm/Kconfig" source "drivers/clk/hisilicon/Kconfig" source "drivers/clk/mvebu/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 46869d6..627da26 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -33,6 +33,7 @@ obj-$(CONFIG_ARCH_MB86S7X) += clk-mb86s7x.o obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o +obj-$(CONFIG_COMMON_CLK_OXNAS) += clk-oxnas.o obj-$(CONFIG_COMMON_CLK_PALMAS) += clk-palmas.o obj-$(CONFIG_CLK_QORIQ) += clk-qoriq.o obj-$(CONFIG_COMMON_CLK_RK808) += clk-rk808.o diff --git a/drivers/clk/clk-oxnas.c b/drivers/clk/clk-oxnas.c new file mode 100644 index 0000000..5f02cfa --- /dev/null +++ b/drivers/clk/clk-oxnas.c @@ -0,0 +1,202 @@ +/* + * Copyright (C) 2010 Broadcom + * Copyright (C) 2012 Stephen Warren + * Copyright (C) 2016 Neil Armstrong <narmstrong@baylibre.com> + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/clk-provider.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/stringify.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> + +/* Standard regmap gate clocks */ +struct clk_oxnas { + struct clk_hw hw; + signed char bit; + struct regmap *regmap; +}; + +/* Regmap offsets */ +#define CLK_STAT_REGOFFSET 0x24 +#define CLK_SET_REGOFFSET 0x2c +#define CLK_CLR_REGOFFSET 0x30 + +static inline struct clk_oxnas *to_clk_oxnas(struct clk_hw *hw) +{ + return container_of(hw, struct clk_oxnas, hw); +} + +static int oxnas_clk_is_enabled(struct clk_hw *hw) +{ + struct clk_oxnas *std = to_clk_oxnas(hw); + int ret; + unsigned int val; + + ret = regmap_read(std->regmap, CLK_STAT_REGOFFSET, &val); + if (ret < 0) + return ret; + + return val & BIT(std->bit); +} + +static int oxnas_clk_enable(struct clk_hw *hw) +{ + struct clk_oxnas *std = to_clk_oxnas(hw); + + regmap_write(std->regmap, CLK_SET_REGOFFSET, BIT(std->bit)); + + return 0; +} + +static void oxnas_clk_disable(struct clk_hw *hw) +{ + struct clk_oxnas *std = to_clk_oxnas(hw); + + regmap_write(std->regmap, CLK_CLR_REGOFFSET, BIT(std->bit)); +} + +static const struct clk_ops oxnas_clk_ops = { + .enable = oxnas_clk_enable, + .disable = oxnas_clk_disable, + .is_enabled = oxnas_clk_is_enabled, +}; + +static const char *const oxnas_clk_parents[] = { + "oscillator", +}; + +static const char *const eth_parents[] = { + "gmacclk", +}; + +#define DECLARE_STD_CLKP(__clk, __parent) \ +static const struct clk_init_data clk_##__clk##_init = { \ + .name = __stringify(__clk), \ + .ops = &oxnas_clk_ops, \ + .parent_names = __parent, \ + .num_parents = ARRAY_SIZE(__parent), \ +} + +#define DECLARE_STD_CLK(__clk) DECLARE_STD_CLKP(__clk, oxnas_clk_parents) + +/* Clk init data declaration */ +DECLARE_STD_CLK(leon); +DECLARE_STD_CLK(dma_sgdma); +DECLARE_STD_CLK(cipher); +DECLARE_STD_CLK(sata); +DECLARE_STD_CLK(audio); +DECLARE_STD_CLK(usbmph); +DECLARE_STD_CLKP(etha, eth_parents); +DECLARE_STD_CLK(pciea); +DECLARE_STD_CLK(nand); + +/* Bit - Name association */ +static const struct clk_init_data *clk_oxnas_init[] = { + [0] = &clk_leon_init, + [1] = &clk_dma_sgdma_init, + [2] = &clk_cipher_init, + [3] = NULL, /* Do not touch to DDR clock */ + [4] = &clk_sata_init, + [5] = &clk_audio_init, + [6] = &clk_usbmph_init, + [7] = &clk_etha_init, + [8] = &clk_pciea_init, + [9] = &clk_nand_init, +}; + +static int oxnas_stdclk_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct regmap *regmap; + struct clk_oxnas *clk_oxnas; + struct clk_onecell_data *onecell_data; + struct clk **clks; + unsigned int clks_count = 0; + int i; + + clk_oxnas = devm_kzalloc(&pdev->dev, + sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init), + GFP_KERNEL); + if (!clk_oxnas) + return -ENOMEM; + + clks = devm_kzalloc(&pdev->dev, + sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init), + GFP_KERNEL); + if (!clks) + return -ENOMEM; + + onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data), + GFP_KERNEL); + if (!onecell_data) + return -ENOMEM; + + regmap = syscon_node_to_regmap(of_get_parent(np)); + if (!regmap) { + dev_err(&pdev->dev, "failed to have parent regmap\n"); + return -EINVAL; + } + + for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) { + struct clk_oxnas *_clk; + + if (!clk_oxnas_init[i]) + continue; + + _clk = &clk_oxnas[i]; + _clk->bit = i; + _clk->hw.init = clk_oxnas_init[i]; + _clk->regmap = regmap; + + clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw); + if (WARN_ON(IS_ERR(clks[clks_count]))) + return PTR_ERR(clks[clks_count]); + + ++clks_count; + } + + onecell_data->clks = clks; + onecell_data->clk_num = clks_count; + + return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data); +} + +static int oxnas_stdclk_remove(struct platform_device *pdev) +{ + of_clk_del_provider(pdev->dev.of_node); + + return 0; +} + +static const struct of_device_id oxnas_stdclk_dt_ids[] = { + { .compatible = "oxsemi,ox810se-stdclk" }, + { } +}; +MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids); + +static struct platform_driver oxnas_stdclk_driver = { + .probe = oxnas_stdclk_probe, + .remove = oxnas_stdclk_remove, + .driver = { + .name = "oxnas-stdclk", + .of_match_table = of_match_ptr(oxnas_stdclk_dt_ids), + }, +}; + +module_platform_driver(oxnas_stdclk_driver);
Add Oxford Semiconductor OXNAS SoC Family Standard Clocks support. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/Kconfig | 6 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-oxnas.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+) create mode 100644 drivers/clk/clk-oxnas.c