Message ID | 20170222150349.16790-2-alexander.stein@systec-electronic.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Stephen Boyd |
Headers | show |
On 02/22, Alexander Stein wrote: > This driver currently only implements the QSPI divider register > SCFG_QSPI_CFG. > > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> Nobody reviewed. Sorry it fell down in my queue and I forgot. > --- > drivers/clk/Makefile | 1 + > drivers/clk/clk-ls1021a.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > create mode 100644 drivers/clk/clk-ls1021a.c > > diff --git a/drivers/clk/clk-ls1021a.c b/drivers/clk/clk-ls1021a.c > new file mode 100644 > index 0000000..2f64806 > --- /dev/null > +++ b/drivers/clk/clk-ls1021a.c > @@ -0,0 +1,58 @@ > +/* > + * Copyright (C) 2017 SYS TEC electronic GmbH > + * Alexander Stein <alexander.stein@systec-electronic.com> > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation. > + */ > +#include <linux/io.h> > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > + > +static const struct clk_div_table qspi_cfg_div_table[] = { > + { 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 }, > + { 0x4, 20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7, 8 }, > + { 0 }, > +}; > + > +static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np) > +{ > + const char *parent_name; > + const char *name; > + void __iomem *base; > + struct clk *parent_clk; > + struct clk *clk; > + struct resource res; Unused? but should be used! > + > + base = of_iomap(np, 0); > + if (!base) { > + pr_warn("Failed to map address range for node %s\n", np->name); We don't typically need any sort of error message. > + return; > + } > + > + parent_clk = of_clk_get(np, 0); > + if (IS_ERR(parent_clk)) { > + pr_warn("Failed to get clock for node %s\n", np->name); > + return; > + } > + > + /* Register the input clock under the desired name. */ > + parent_name = __clk_get_name(parent_clk); > + > + if (of_property_read_string(np, "clock-output-names", &name)) > + name = np->name; Please just hardcode it unless you really need different names from DT. We're trying to move away from clock-output-names for clk tree description as it's inflexible in the face of DT ABI. > + > + /* Works only as those 4 bits (Bits 28-31 big endian) do not cross byte boundary */ This line is too long, but also what's going on? Some sort of 64-bit register here? > + clk = clk_register_divider_table(NULL, name, parent_name, > + 0, base, > + 4, 4, 0, qspi_cfg_div_table, NULL); > + if (IS_ERR(clk)) { > + pr_warn("Failed to register divider table clock (%ld)\n", PTR_ERR(clk)); > + return; > + } > + of_clk_add_provider(np, of_clk_src_simple_get, clk); Can you please use the clk_hw based provider and divider registration APIs? > +} > +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", scfg_qspi_cfg_ls1021a_init); Can this be a platform driver? We usually reserve CLK_OF_DECLARE for clks that we need to get the timer or interrupt controllers working because they need to probe so early. Otherwise use a proper driver and we can use platform device APIs instead of OF specific ones to map register regions and get clks.
On Friday 07 April 2017 12:33:24, Stephen Boyd wrote: > On 02/22, Alexander Stein wrote: > > +static const struct clk_div_table qspi_cfg_div_table[] = { > > + { 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 }, > > + { 0x4, 20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7, 8 }, > > + { 0 }, > > +}; > > + > > +static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np) > > +{ > > + const char *parent_name; > > + const char *name; > > + void __iomem *base; > > + struct clk *parent_clk; > > + struct clk *clk; > > + struct resource res; > > Unused? but should be used! Just copy & paste mistake. It can be removed. > > + > > + base = of_iomap(np, 0); > > + if (!base) { > > + pr_warn("Failed to map address range for node %s\n", np->name); > > We don't typically need any sort of error message. Ok. > > + return; > > + } > > + > > + parent_clk = of_clk_get(np, 0); > > + if (IS_ERR(parent_clk)) { > > + pr_warn("Failed to get clock for node %s\n", np->name); > > + return; > > + } > > + > > + /* Register the input clock under the desired name. */ > > + parent_name = __clk_get_name(parent_clk); > > + > > + if (of_property_read_string(np, "clock-output-names", &name)) > > + name = np->name; > > Please just hardcode it unless you really need different names > from DT. We're trying to move away from clock-output-names for > clk tree description as it's inflexible in the face of DT ABI. But how do you then reference this clock from another DT node if clock-output- names is remove dfrom patch 2/3? See path 3/3. I have to admit I'm not an expert on DT clocks. > > + > > + /* Works only as those 4 bits (Bits 28-31 big endian) do not cross byte > > boundary */ > This line is too long, but also what's going on? Some sort of > 64-bit register here? No, this is periphery attached as big-endian on a little-endian CPU, the infamous LS1021A has lots (but not all) of them. Finally this needs a proper clk improvement to support big-endian accesses on little-endian CPUs. Don't look at it (in detail), yet. > > + clk = clk_register_divider_table(NULL, name, parent_name, > > + 0, base, > > + 4, 4, 0, qspi_cfg_div_table, NULL); > > + if (IS_ERR(clk)) { > > + pr_warn("Failed to register divider table clock (%ld)\n", > > PTR_ERR(clk)); > > + return; > > + } > > + of_clk_add_provider(np, of_clk_src_simple_get, clk); > > Can you please use the clk_hw based provider and divider > registration APIs? Is there a specific reason to use clk_hw based API? Both apparently do the same. > > +} > > +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", > > scfg_qspi_cfg_ls1021a_init); > Can this be a platform driver? We usually reserve CLK_OF_DECLARE > for clks that we need to get the timer or interrupt controllers > working because they need to probe so early. Otherwise use a > proper driver and we can use platform device APIs instead of OF > specific ones to map register regions and get clks. As this is a simple clock divider neither timers nor interrupts are involved. There should be no problem to change this to a platform driver changing the init function into a probe one. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12, Alexander Stein wrote: > On Friday 07 April 2017 12:33:24, Stephen Boyd wrote: > > On 02/22, Alexander Stein wrote: > > > + return; > > > + } > > > + > > > + parent_clk = of_clk_get(np, 0); > > > + if (IS_ERR(parent_clk)) { > > > + pr_warn("Failed to get clock for node %s\n", np->name); > > > + return; > > > + } > > > + > > > + /* Register the input clock under the desired name. */ > > > + parent_name = __clk_get_name(parent_clk); > > > + > > > + if (of_property_read_string(np, "clock-output-names", &name)) > > > + name = np->name; > > > > Please just hardcode it unless you really need different names > > from DT. We're trying to move away from clock-output-names for > > clk tree description as it's inflexible in the face of DT ABI. > > But how do you then reference this clock from another DT node if clock-output- > names is remove dfrom patch 2/3? See path 3/3. I have to admit I'm not an > expert on DT clocks. clocks = <&phandle cells...>? I'm not sure I follow the problem here? clock-output-names is optional. > > > > + > > > + /* Works only as those 4 bits (Bits 28-31 big endian) do not cross > byte > > > boundary */ > > This line is too long, but also what's going on? Some sort of > > 64-bit register here? > > No, this is periphery attached as big-endian on a little-endian CPU, the > infamous LS1021A has lots (but not all) of them. Finally this needs a proper > clk improvement to support big-endian accesses on little-endian CPUs. Don't > look at it (in detail), yet. > > > > + clk = clk_register_divider_table(NULL, name, parent_name, > > > + 0, base, > > > + 4, 4, 0, qspi_cfg_div_table, NULL); > > > + if (IS_ERR(clk)) { > > > + pr_warn("Failed to register divider table clock (%ld)\n", > > > PTR_ERR(clk)); > > > + return; > > > + } > > > + of_clk_add_provider(np, of_clk_src_simple_get, clk); > > > > Can you please use the clk_hw based provider and divider > > registration APIs? > > Is there a specific reason to use clk_hw based API? Both apparently do the > same. We're trying to split the consumer and provider APIs along struct clk_hw and struct clk respectively. If we can have drivers only registers clk_hw pointers and never get back anything but an error code, then we can force consumers to always go through the clk_get() family of APIs. Then we can easily tell who is a provider, who is a consumer, and who is a provider + a consumer. Right now this isn't always clear cut because clk_hw has access to struct clk, and also clk_regsiter() returns a clk pointer, but it doesn't really get used by anything in a provider driver, unless provider drivers are doing something with the consumer API. > > > > +} > > > +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", > > > scfg_qspi_cfg_ls1021a_init); > > Can this be a platform driver? We usually reserve CLK_OF_DECLARE > > for clks that we need to get the timer or interrupt controllers > > working because they need to probe so early. Otherwise use a > > proper driver and we can use platform device APIs instead of OF > > specific ones to map register regions and get clks. > > As this is a simple clock divider neither timers nor interrupts are involved. > There should be no problem to change this to a platform driver changing the > init function into a probe one. > Sounds great! Please do.
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 925081e..611f53f 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o obj-$(CONFIG_COMMON_CLK_CS2000_CP) += clk-cs2000-cp.o obj-$(CONFIG_ARCH_EFM32) += clk-efm32gg.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o +obj-$(CONFIG_SOC_LS1021A) += clk-ls1021a.o obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o obj-$(CONFIG_ARCH_MB86S7X) += clk-mb86s7x.o obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o diff --git a/drivers/clk/clk-ls1021a.c b/drivers/clk/clk-ls1021a.c new file mode 100644 index 0000000..2f64806 --- /dev/null +++ b/drivers/clk/clk-ls1021a.c @@ -0,0 +1,58 @@ +/* + * Copyright (C) 2017 SYS TEC electronic GmbH + * Alexander Stein <alexander.stein@systec-electronic.com> + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation. + */ +#include <linux/io.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/of.h> +#include <linux/of_address.h> + +static const struct clk_div_table qspi_cfg_div_table[] = { + { 0x0, 256 }, { 0x1, 64 }, { 0x2, 32 }, { 0x3, 24 }, + { 0x4, 20 }, { 0x5, 15 }, { 0x6, 12 }, { 0x7, 8 }, + { 0 }, +}; + +static void __init scfg_qspi_cfg_ls1021a_init(struct device_node *np) +{ + const char *parent_name; + const char *name; + void __iomem *base; + struct clk *parent_clk; + struct clk *clk; + struct resource res; + + base = of_iomap(np, 0); + if (!base) { + pr_warn("Failed to map address range for node %s\n", np->name); + return; + } + + parent_clk = of_clk_get(np, 0); + if (IS_ERR(parent_clk)) { + pr_warn("Failed to get clock for node %s\n", np->name); + return; + } + + /* Register the input clock under the desired name. */ + parent_name = __clk_get_name(parent_clk); + + if (of_property_read_string(np, "clock-output-names", &name)) + name = np->name; + + /* Works only as those 4 bits (Bits 28-31 big endian) do not cross byte boundary */ + clk = clk_register_divider_table(NULL, name, parent_name, + 0, base, + 4, 4, 0, qspi_cfg_div_table, NULL); + if (IS_ERR(clk)) { + pr_warn("Failed to register divider table clock (%ld)\n", PTR_ERR(clk)); + return; + } + of_clk_add_provider(np, of_clk_src_simple_get, clk); +} +CLK_OF_DECLARE(scfg_qspi_cfg_ls1021a, "fsl,scfg-qspi-cfg-ls1021a", scfg_qspi_cfg_ls1021a_init);
This driver currently only implements the QSPI divider register SCFG_QSPI_CFG. Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> --- drivers/clk/Makefile | 1 + drivers/clk/clk-ls1021a.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 drivers/clk/clk-ls1021a.c