| Message ID | 20191209233305.18619-2-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: clock: document the fsl-sai driver | expand |
Quoting Michael Walle (2019-12-09 15:33:05) > diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c > new file mode 100644 > index 000000000000..b92054d15ab1 > --- /dev/null > +++ b/drivers/clk/clk-fsl-sai.c > @@ -0,0 +1,84 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Freescale SAI BCLK as a generic clock driver > + * > + * Copyright 2019 Kontron Europe GmbH > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/err.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/slab.h> > + > +#define I2S_CSR 0x00 > +#define I2S_CR2 0x08 > +#define CSR_BCE_BIT 28 > +#define CR2_BCD BIT(24) > +#define CR2_DIV_SHIFT 0 > +#define CR2_DIV_WIDTH 8 > + > +struct fsl_sai_clk { > + struct clk_divider div; > + struct clk_gate gate; > + spinlock_t lock; > +}; > + > +static void __init fsl_sai_clk_setup(struct device_node *node) > +{ > + const char *clk_name = node->name; > + struct fsl_sai_clk *sai_clk; > + unsigned int num_parents; > + const char *parent_name; > + void __iomem *base; > + struct clk_hw *hw; > + > + num_parents = of_clk_get_parent_count(node); > + if (!num_parents) { > + pr_err("%s: no parent found", clk_name); > + return; > + } > + > + parent_name = of_clk_get_parent_name(node, 0); Could this use the new way of specifying clk parents so that we don't have to query DT for parent names and just let the core framework do it whenever it needs to? > + > + sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL); > + if (!sai_clk) > + return; > + > + base = of_iomap(node, 0); > + if (base == NULL) { > + pr_err("%s: failed to map register space", clk_name); > + goto err; > + } > + > + spin_lock_init(&sai_clk->lock); > + > + sai_clk->gate.reg = base + I2S_CSR; > + sai_clk->gate.bit_idx = CSR_BCE_BIT; > + sai_clk->gate.lock = &sai_clk->lock; > + > + sai_clk->div.reg = base + I2S_CR2; > + sai_clk->div.shift = CR2_DIV_SHIFT; > + sai_clk->div.width = CR2_DIV_WIDTH; > + sai_clk->div.lock = &sai_clk->lock; > + > + /* set clock direction, we are the BCLK master */ Should this configuration come from DT somehow? > + writel(CR2_BCD, base + I2S_CR2); > + > + hw = clk_hw_register_composite(NULL, clk_name, &parent_name, 1, > + NULL, NULL, > + &sai_clk->div.hw, &clk_divider_ops, > + &sai_clk->gate.hw, &clk_gate_ops, > + CLK_SET_RATE_GATE); > + if (IS_ERR(hw)) > + goto err; > + > + of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw); > + > + return; > + > +err: > + kfree(sai_clk); > +} > + > +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", fsl_sai_clk_setup); Is there a reason this can't be a platform device driver?
Hi Stephen, thanks for the review. Am 2019-12-24 09:05, schrieb Stephen Boyd: > Quoting Michael Walle (2019-12-09 15:33:05) >> diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c >> new file mode 100644 >> index 000000000000..b92054d15ab1 >> --- /dev/null >> +++ b/drivers/clk/clk-fsl-sai.c >> @@ -0,0 +1,84 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Freescale SAI BCLK as a generic clock driver >> + * >> + * Copyright 2019 Kontron Europe GmbH >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/err.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/slab.h> >> + >> +#define I2S_CSR 0x00 >> +#define I2S_CR2 0x08 >> +#define CSR_BCE_BIT 28 >> +#define CR2_BCD BIT(24) >> +#define CR2_DIV_SHIFT 0 >> +#define CR2_DIV_WIDTH 8 >> + >> +struct fsl_sai_clk { >> + struct clk_divider div; >> + struct clk_gate gate; >> + spinlock_t lock; >> +}; >> + >> +static void __init fsl_sai_clk_setup(struct device_node *node) >> +{ >> + const char *clk_name = node->name; >> + struct fsl_sai_clk *sai_clk; >> + unsigned int num_parents; >> + const char *parent_name; >> + void __iomem *base; >> + struct clk_hw *hw; >> + >> + num_parents = of_clk_get_parent_count(node); >> + if (!num_parents) { >> + pr_err("%s: no parent found", clk_name); >> + return; >> + } >> + >> + parent_name = of_clk_get_parent_name(node, 0); > > Could this use the new way of specifying clk parents so that we don't > have to query DT for parent names and just let the core framework do it > whenever it needs to? you mean specifying parent_data with .index = 0? Seems like clk_composite does not support this. The parent can only be specified by supplying the clock names. I could add that in a separate patch. What do you think about the following new functions, where a driver can use parent_data instead of parent_names. +struct clk *clk_register_composite_pdata(struct device *dev, const char *name, + const struct clk_parent_data *parent_data, + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, + unsigned long flags); +struct clk_hw *clk_hw_register_composite_pdata(struct device *dev, + const char *name, const struct clk_parent_data *parent_data, + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, + unsigned long flags); >> + >> + sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL); >> + if (!sai_clk) >> + return; >> + >> + base = of_iomap(node, 0); >> + if (base == NULL) { >> + pr_err("%s: failed to map register space", clk_name); >> + goto err; >> + } >> + >> + spin_lock_init(&sai_clk->lock); >> + >> + sai_clk->gate.reg = base + I2S_CSR; >> + sai_clk->gate.bit_idx = CSR_BCE_BIT; >> + sai_clk->gate.lock = &sai_clk->lock; >> + >> + sai_clk->div.reg = base + I2S_CR2; >> + sai_clk->div.shift = CR2_DIV_SHIFT; >> + sai_clk->div.width = CR2_DIV_WIDTH; >> + sai_clk->div.lock = &sai_clk->lock; >> + >> + /* set clock direction, we are the BCLK master */ > > Should this configuration come from DT somehow? No, we are always master, because as a slave, there would be no clock output ;) >> + writel(CR2_BCD, base + I2S_CR2); >> + >> + hw = clk_hw_register_composite(NULL, clk_name, &parent_name, >> 1, >> + NULL, NULL, >> + &sai_clk->div.hw, >> &clk_divider_ops, >> + &sai_clk->gate.hw, >> &clk_gate_ops, >> + CLK_SET_RATE_GATE); >> + if (IS_ERR(hw)) >> + goto err; >> + >> + of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw); >> + >> + return; >> + >> +err: >> + kfree(sai_clk); >> +} >> + >> +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", >> fsl_sai_clk_setup); > > Is there a reason this can't be a platform device driver? I don't think so, the user will be a sound codec for now. I'll convert it to a platform device, in that case I could also use the devm_ variants. -michael
Quoting Michael Walle (2020-01-01 07:15:32) > > Hi Stephen, > > thanks for the review. > > Am 2019-12-24 09:05, schrieb Stephen Boyd: > > Quoting Michael Walle (2019-12-09 15:33:05) > >> diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c > >> new file mode 100644 > >> index 000000000000..b92054d15ab1 > >> --- /dev/null > >> +++ b/drivers/clk/clk-fsl-sai.c > >> @@ -0,0 +1,84 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Freescale SAI BCLK as a generic clock driver > >> + * > >> + * Copyright 2019 Kontron Europe GmbH > >> + */ > >> + > >> +#include <linux/clk-provider.h> > >> +#include <linux/err.h> > >> +#include <linux/of.h> > >> +#include <linux/of_address.h> > >> +#include <linux/slab.h> > >> + > >> +#define I2S_CSR 0x00 > >> +#define I2S_CR2 0x08 > >> +#define CSR_BCE_BIT 28 > >> +#define CR2_BCD BIT(24) > >> +#define CR2_DIV_SHIFT 0 > >> +#define CR2_DIV_WIDTH 8 > >> + > >> +struct fsl_sai_clk { > >> + struct clk_divider div; > >> + struct clk_gate gate; > >> + spinlock_t lock; > >> +}; > >> + > >> +static void __init fsl_sai_clk_setup(struct device_node *node) > >> +{ > >> + const char *clk_name = node->name; > >> + struct fsl_sai_clk *sai_clk; > >> + unsigned int num_parents; > >> + const char *parent_name; > >> + void __iomem *base; > >> + struct clk_hw *hw; > >> + > >> + num_parents = of_clk_get_parent_count(node); > >> + if (!num_parents) { > >> + pr_err("%s: no parent found", clk_name); > >> + return; > >> + } > >> + > >> + parent_name = of_clk_get_parent_name(node, 0); > > > > Could this use the new way of specifying clk parents so that we don't > > have to query DT for parent names and just let the core framework do it > > whenever it needs to? > > you mean specifying parent_data with .index = 0? Seems like > clk_composite > does not support this. The parent can only be specified by supplying the > clock names. > > I could add that in a separate patch. What do you think about the > following new functions, where a driver can use parent_data instead > of parent_names. I started doing this in https://lkml.kernel.org/r/20190830150923.259497-1-sboyd@kernel.org but I never got around to the composite clks. Sounds fine to add this new API for your use case. > > +struct clk *clk_register_composite_pdata(struct device *dev, const char > *name, > + const struct clk_parent_data *parent_data, > + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, > + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, > + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, > + unsigned long flags); > > +struct clk_hw *clk_hw_register_composite_pdata(struct device *dev, > + const char *name, const struct clk_parent_data > *parent_data, > + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, > + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, > + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, > + unsigned long flags); > > > >> + > >> + sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL); > >> + if (!sai_clk) > >> + return; > >> + > >> + base = of_iomap(node, 0); > >> + if (base == NULL) { > >> + pr_err("%s: failed to map register space", clk_name); > >> + goto err; > >> + } > >> + > >> + spin_lock_init(&sai_clk->lock); > >> + > >> + sai_clk->gate.reg = base + I2S_CSR; > >> + sai_clk->gate.bit_idx = CSR_BCE_BIT; > >> + sai_clk->gate.lock = &sai_clk->lock; > >> + > >> + sai_clk->div.reg = base + I2S_CR2; > >> + sai_clk->div.shift = CR2_DIV_SHIFT; > >> + sai_clk->div.width = CR2_DIV_WIDTH; > >> + sai_clk->div.lock = &sai_clk->lock; > >> + > >> + /* set clock direction, we are the BCLK master */ > > > > Should this configuration come from DT somehow? > > No, we are always master, because as a slave, there would be no clock > output ;) Got it. > > >> + writel(CR2_BCD, base + I2S_CR2); > >> + > >> + hw = clk_hw_register_composite(NULL, clk_name, &parent_name, > >> 1, > >> + NULL, NULL, > >> + &sai_clk->div.hw, > >> &clk_divider_ops, > >> + &sai_clk->gate.hw, > >> &clk_gate_ops, > >> + CLK_SET_RATE_GATE); > >> + if (IS_ERR(hw)) > >> + goto err; > >> + > >> + of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw); > >> + > >> + return; > >> + > >> +err: > >> + kfree(sai_clk); > >> +} > >> + > >> +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", > >> fsl_sai_clk_setup); > > > > Is there a reason this can't be a platform device driver? > > I don't think so, the user will be a sound codec for now. I'll convert > it > to a platform device, in that case I could also use the devm_ variants. > Awesome. Thanks!
Hi Stephen, >> >> + parent_name = of_clk_get_parent_name(node, 0); >> > >> > Could this use the new way of specifying clk parents so that we don't >> > have to query DT for parent names and just let the core framework do it >> > whenever it needs to? >> >> you mean specifying parent_data with .index = 0? Seems like >> clk_composite >> does not support this. The parent can only be specified by supplying >> the >> clock names. >> >> I could add that in a separate patch. What do you think about the >> following new functions, where a driver can use parent_data instead >> of parent_names. > > I started doing this in > https://lkml.kernel.org/r/20190830150923.259497-1-sboyd@kernel.org but > I > never got around to the composite clks. Sounds fine to add this new API > for your use case. Yeah took me a while to figure out what you've meant by the "new way" ;) Anyway, I've posted a v3 of this series with the new composite clock API. > >> >> +struct clk *clk_register_composite_pdata(struct device *dev, const >> char >> *name, >> + const struct clk_parent_data *parent_data, >> + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, >> + struct clk_hw *rate_hw, const struct clk_ops >> *rate_ops, >> + struct clk_hw *gate_hw, const struct clk_ops >> *gate_ops, >> + unsigned long flags); num_parents was missing here. added that in the v3. -michael
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 45653a0e6ecd..dd1a5abc4ce8 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -174,6 +174,18 @@ config COMMON_CLK_CS2000_CP help If you say yes here you get support for the CS2000 clock multiplier. +config COMMON_CLK_FSL_SAI + bool "Clock driver for BCLK of Freescale SAI cores" + depends on ARCH_LAYERSCAPE || COMPILE_TEST + help + This driver supports the Freescale SAI (Synchronous Audio Interface) + to be used as a generic clock output. Some SoCs have restrictions + regarding the possible pin multiplexer settings. Eg. on some SoCs + two SAI interfaces can only be enabled together. If just one is + needed, the BCLK pin of the second one can be used as general + purpose clock output. Ideally, it can be used to drive an audio + codec (sometimes known as MCLK). + config COMMON_CLK_GEMINI bool "Clock driver for Cortina Systems Gemini SoC" depends on ARCH_GEMINI || COMPILE_TEST diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 0696a0c1ab58..ec23fd956228 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -29,6 +29,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_COMMON_CLK_FIXED_MMIO) += clk-fixed-mmio.o +obj-$(CONFIG_COMMON_CLK_FSL_SAI) += clk-fsl-sai.o obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o obj-$(CONFIG_COMMON_CLK_ASPEED) += clk-aspeed.o obj-$(CONFIG_MACH_ASPEED_G6) += clk-ast2600.o diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c new file mode 100644 index 000000000000..b92054d15ab1 --- /dev/null +++ b/drivers/clk/clk-fsl-sai.c @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Freescale SAI BCLK as a generic clock driver + * + * Copyright 2019 Kontron Europe GmbH + */ + +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/slab.h> + +#define I2S_CSR 0x00 +#define I2S_CR2 0x08 +#define CSR_BCE_BIT 28 +#define CR2_BCD BIT(24) +#define CR2_DIV_SHIFT 0 +#define CR2_DIV_WIDTH 8 + +struct fsl_sai_clk { + struct clk_divider div; + struct clk_gate gate; + spinlock_t lock; +}; + +static void __init fsl_sai_clk_setup(struct device_node *node) +{ + const char *clk_name = node->name; + struct fsl_sai_clk *sai_clk; + unsigned int num_parents; + const char *parent_name; + void __iomem *base; + struct clk_hw *hw; + + num_parents = of_clk_get_parent_count(node); + if (!num_parents) { + pr_err("%s: no parent found", clk_name); + return; + } + + parent_name = of_clk_get_parent_name(node, 0); + + sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL); + if (!sai_clk) + return; + + base = of_iomap(node, 0); + if (base == NULL) { + pr_err("%s: failed to map register space", clk_name); + goto err; + } + + spin_lock_init(&sai_clk->lock); + + sai_clk->gate.reg = base + I2S_CSR; + sai_clk->gate.bit_idx = CSR_BCE_BIT; + sai_clk->gate.lock = &sai_clk->lock; + + sai_clk->div.reg = base + I2S_CR2; + sai_clk->div.shift = CR2_DIV_SHIFT; + sai_clk->div.width = CR2_DIV_WIDTH; + sai_clk->div.lock = &sai_clk->lock; + + /* set clock direction, we are the BCLK master */ + writel(CR2_BCD, base + I2S_CR2); + + hw = clk_hw_register_composite(NULL, clk_name, &parent_name, 1, + NULL, NULL, + &sai_clk->div.hw, &clk_divider_ops, + &sai_clk->gate.hw, &clk_gate_ops, + CLK_SET_RATE_GATE); + if (IS_ERR(hw)) + goto err; + + of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw); + + return; + +err: + kfree(sai_clk); +} + +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", fsl_sai_clk_setup);
With this driver it is possible to use the BCLK pin of the SAI module as a generic clock output. This is esp. useful if you want to drive a clock to an audio codec. Because the output only allows integer divider values the audio codec needs an integrated PLL. Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v1: - none drivers/clk/Kconfig | 12 ++++++ drivers/clk/Makefile | 1 + drivers/clk/clk-fsl-sai.c | 84 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 drivers/clk/clk-fsl-sai.c