| Message ID | 20191209233305.18619-1-michael@walle.cc |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: clock: document the fsl-sai driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 1 warnings, 55 lines checked" |
| robh/dt-meta-schema | success |
On Mon, Dec 9, 2019 at 5:33 PM Michael Walle <michael@walle.cc> wrote: > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > changes since v1: > - dual license gpl-2.0-only and bsd-2-clause > - add "additionalProperties: false" > - wrap example in soc {} node with correct #address-cells and #size-cells > > .../bindings/clock/fsl,sai-clock.yaml | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/fsl,sai-clock.yaml Reviewed-by: Rob Herring <robh@kernel.org>
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/Documentation/devicetree/bindings/clock/fsl,sai-clock.yaml b/Documentation/devicetree/bindings/clock/fsl,sai-clock.yaml new file mode 100644 index 000000000000..8fb2060ac47f --- /dev/null +++ b/Documentation/devicetree/bindings/clock/fsl,sai-clock.yaml @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/bindings/clock/fsl,sai-clock.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Freescale SAI bitclock-as-a-clock binding + +maintainers: + - Michael Walle <michael@walle.cc> + +description: | + It is possible to use the BCLK pin of a SAI module as a generic clock + output. Some SoC are very constrained in their pin multiplexer + configuration. Eg. pins can only be changed groups. For example, on the + LS1028A SoC you can only enable SAIs in pairs. If you use only one SAI, + the second pins are wasted. Using this binding it is possible to use the + clock of the second SAI as a MCLK clock for an audio codec, for example. + + This is a composite of a gated clock and a divider clock. + +properties: + compatible: + const: fsl,vf610-sai-clock + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + '#clock-cells': + const: 0 + +required: + - compatible + - reg + - clocks + - '#clock-cells' + +additionalProperties: false + +examples: + - | + soc { + #address-cells = <2>; + #size-cells = <2>; + + mclk: clock-mclk@f130080 { + compatible = "fsl,vf610-sai-clock"; + reg = <0x0 0xf130080 0x0 0x80>; + #clock-cells = <0>; + clocks = <&parentclk>; + }; + };
Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v1: - dual license gpl-2.0-only and bsd-2-clause - add "additionalProperties: false" - wrap example in soc {} node with correct #address-cells and #size-cells .../bindings/clock/fsl,sai-clock.yaml | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/fsl,sai-clock.yaml