[alsa-devel] [PATCH 0/5] ASOC: bcm2835: move bcm2835-i2s to use clock framework
From: Martin Sperl kernel@martin.sperl.org
This patchset enables the bcm2835-i2s driver to use the clock framework which was introduced with commit 94cb7f76caa0b337 ("Switch to using the new clock driver support").
This commit resulted in the fact that the bcm2835-i2s driver was no longer working due to some register addresses used by 2 drivers (clk-bcm2835 and bcm2835-i2s).
To make it all possible this also required the introduction of the PCM clock into the clk-bcm2835 driver. This patchset relies on the patch by Remi Pommarel repk@triplefau.lt that introduces the ability to set parent clocks ("clk: bcm2835: Support for clock parent selection"), which is (as far as I understood) in clk-next and slated for 4.5.
Note that there is one regression: the clk-bcm2835 does not yet support the mash functionality which the SOC-Hw supports, this may result in slightly more "audiable noise" than the original driver. But as this is more about making the driver functional again, this is - I believe - a drawback we can accept for now.
Martin Sperl (5): ASoC: bcm2835: cleanup includes by ordering them alphabetically clk: bcm2835: enable management of PCM clock ASoC: bcm2835: move to use the clock framework ARM: bcm2835: I2S: use new register-range and clock framework dt-bindings: bsm2835: fix bindings documentation to use new clock framework
.../devicetree/bindings/sound/brcm,bcm2835-i2s.txt | 7 +- arch/arm/boot/dts/bcm2835.dtsi | 5 +- drivers/clk/bcm/clk-bcm2835.c | 15 + include/dt-bindings/clock/bcm2835.h | 3 +- sound/soc/bcm/bcm2835-i2s.c | 293 +++++--------------- 5 files changed, 91 insertions(+), 232 deletions(-)
-- 1.7.10.4
From: Martin Sperl kernel@martin.sperl.org
Cleanup of includes so that they are ordered alphabetically.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- sound/soc/bcm/bcm2835-i2s.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index 8c435be..3303d5f 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -31,20 +31,20 @@ * General Public License for more details. */
+#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> #include <linux/init.h> +#include <linux/io.h> #include <linux/module.h> -#include <linux/device.h> #include <linux/slab.h> -#include <linux/delay.h> -#include <linux/io.h> -#include <linux/clk.h>
#include <sound/core.h> +#include <sound/dmaengine_pcm.h> +#include <sound/initval.h> #include <sound/pcm.h> #include <sound/pcm_params.h> -#include <sound/initval.h> #include <sound/soc.h> -#include <sound/dmaengine_pcm.h>
/* Clock registers */ #define BCM2835_CLK_PCMCTL_REG 0x00
From: Martin Sperl kernel@martin.sperl.org
Enable the PCM clock in the SOC, which is used by the bcm2835-i2s driver.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- drivers/clk/bcm/clk-bcm2835.c | 15 +++++++++++++++ include/dt-bindings/clock/bcm2835.h | 3 ++- 2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index 015e687..5d45457 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -88,6 +88,8 @@ #define CM_HSMDIV 0x08c #define CM_OTPCTL 0x090 #define CM_OTPDIV 0x094 +#define CM_PCMCTL 0x098 +#define CM_PCMDIV 0x09c #define CM_PWMCTL 0x0a0 #define CM_PWMDIV 0x0a4 #define CM_SMICTL 0x0b0 @@ -817,6 +819,16 @@ static const struct bcm2835_clock_data bcm2835_clock_pwm_data = { .frac_bits = 12, };
+static const struct bcm2835_clock_data bcm2835_clock_pcm_data = { + .name = "pcm", + .num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents), + .parents = bcm2835_clock_per_parents, + .ctl_reg = CM_PCMCTL, + .div_reg = CM_PCMDIV, + .int_bits = 12, + .frac_bits = 12, +}; + struct bcm2835_pll { struct clk_hw hw; struct bcm2835_cprman *cprman; @@ -1515,6 +1527,7 @@ static int bcm2835_clk_probe(struct platform_device *pdev) cprman->regs = devm_ioremap_resource(dev, res); if (IS_ERR(cprman->regs)) return PTR_ERR(cprman->regs); + pr_info("CLK: %pK %zx\n", cprman->regs, res->start);
cprman->osc_name = of_clk_get_parent_name(dev->of_node, 0); if (!cprman->osc_name) @@ -1581,6 +1594,8 @@ static int bcm2835_clk_probe(struct platform_device *pdev) bcm2835_register_clock(cprman, &bcm2835_clock_hsm_data); clks[BCM2835_CLOCK_EMMC] = bcm2835_register_clock(cprman, &bcm2835_clock_emmc_data); + clks[BCM2835_CLOCK_PCM] = + bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data);
/* * CM_PERIICTL (and CM_PERIACTL, CM_SYSCTL and CM_VPUCTL if diff --git a/include/dt-bindings/clock/bcm2835.h b/include/dt-bindings/clock/bcm2835.h index 61f1d20..0ec850e 100644 --- a/include/dt-bindings/clock/bcm2835.h +++ b/include/dt-bindings/clock/bcm2835.h @@ -44,5 +44,6 @@ #define BCM2835_CLOCK_EMMC 28 #define BCM2835_CLOCK_PERI_IMAGE 29 #define BCM2835_CLOCK_PWM 30 +#define BCM2835_CLOCK_PCM 31
-#define BCM2835_CLOCK_COUNT 31 +#define BCM2835_CLOCK_COUNT 32
On Saturday 09 January 2016 09:25:54 kernel@martin.sperl.org wrote:
--- a/include/dt-bindings/clock/bcm2835.h +++ b/include/dt-bindings/clock/bcm2835.h @@ -44,5 +44,6 @@ #define BCM2835_CLOCK_EMMC 28 #define BCM2835_CLOCK_PERI_IMAGE 29 #define BCM2835_CLOCK_PWM 30 +#define BCM2835_CLOCK_PCM 31
-#define BCM2835_CLOCK_COUNT 31 +#define BCM2835_CLOCK_COUNT 32
The last line contains an incompatible change, please don't do that. If you have to add another clock, do that after the BCM2835_CLOCK_COUNT definition to avoid changing dts files that use that number.
Arnd
Hi Arnd,
On Sat, Jan 9, 2016 at 9:56 PM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 09 January 2016 09:25:54 kernel@martin.sperl.org wrote:
--- a/include/dt-bindings/clock/bcm2835.h +++ b/include/dt-bindings/clock/bcm2835.h @@ -44,5 +44,6 @@ #define BCM2835_CLOCK_EMMC 28 #define BCM2835_CLOCK_PERI_IMAGE 29 #define BCM2835_CLOCK_PWM 30 +#define BCM2835_CLOCK_PCM 31
-#define BCM2835_CLOCK_COUNT 31 +#define BCM2835_CLOCK_COUNT 32
The last line contains an incompatible change, please don't do that. If you have to add another clock, do that after the BCM2835_CLOCK_COUNT definition to avoid changing dts files that use that number.
While I agree this changes dts files (in an unexpected way?), not updating BCM2835_CLOCK_COUNT makes it definition useless. Which teaches that having such definitions in DT headers is not a good idea in the first place...
Hence it can better be replaced (it seems to be unused in dts files, but you can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. This requires changing the driver to e.g. initialize clks[] in bcm2835_clk_probe() based on a table instead of explicit code.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10.01.2016, at 10:30, Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Arnd,
On Sat, Jan 9, 2016 at 9:56 PM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 09 January 2016 09:25:54 kernel@martin.sperl.org wrote:
--- a/include/dt-bindings/clock/bcm2835.h +++ b/include/dt-bindings/clock/bcm2835.h @@ -44,5 +44,6 @@ #define BCM2835_CLOCK_EMMC 28 #define BCM2835_CLOCK_PERI_IMAGE 29 #define BCM2835_CLOCK_PWM 30 +#define BCM2835_CLOCK_PCM 31
-#define BCM2835_CLOCK_COUNT 31 +#define BCM2835_CLOCK_COUNT 32
The last line contains an incompatible change, please don't do that. If you have to add another clock, do that after the BCM2835_CLOCK_COUNT definition to avoid changing dts files that use that number.
While I agree this changes dts files (in an unexpected way?), not updating BCM2835_CLOCK_COUNT makes it definition useless. Which teaches that having such definitions in DT headers is not a good idea in the first place...
Hence it can better be replaced (it seems to be unused in dts files, but you can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. This requires changing the driver to e.g. initialize clks[] in bcm2835_clk_probe() based on a table instead of explicit code.
That is a more general issue with the clock driver (and there have been already some discussions around this when implementing the PWM clock.
So if someone with a better idea how to keep those dt-binding includes synchronized with the definitions used by the code step forward and propose a better solution how to get that implemented?
I guess there will be a few more occurrences of clocks that are currently not defined, which will need to get introduced in the future PWM and PCM were just the last in this series.
Thanks, Martin
On Sun, Jan 10, 2016 at 11:55:48AM +0100, Martin Sperl wrote:
So if someone with a better idea how to keep those dt-binding includes synchronized with the definitions used by the code step forward and propose a better solution how to get that implemented?
I guess there will be a few more occurrences of clocks that are currently not defined, which will need to get introduced in the future PWM and PCM were just the last in this series.
Presumably just making the code not rely on having a define for the number of clocks would deal with the problem (eg, using ARRAY_SIZE internally).
On 10.01.2016, at 12:58, Mark Brown broonie@kernel.org wrote:
On Sun, Jan 10, 2016 at 11:55:48AM +0100, Martin Sperl wrote:
So if someone with a better idea how to keep those dt-binding includes synchronized with the definitions used by the code step forward and propose a better solution how to get that implemented?
I guess there will be a few more occurrences of clocks that are currently not defined, which will need to get introduced in the future PWM and PCM were just the last in this series.
Presumably just making the code not rely on having a define for the number of clocks would deal with the problem (eg, using ARRAY_SIZE internally).
ARRAY_SIZE would work fine, but the code is:
#include <dt-bindings/clock/bcm2835.h> ... struct bcm2835_cprman { struct device *dev; void __iomem *regs; spinlock_t regs_lock; const char *osc_name;
struct clk_onecell_data onecell; struct clk *clks[BCM2835_CLOCK_COUNT]; }; ... static int bcm2835_clk_probe(struct platform_device *pdev) { ... clks[BCM2835_PLLA_CORE] = bcm2835_register_pll_divider(cprman, &bcm2835_plla_core_data); ... clks[BCM2835_CLOCK_PCM] = bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data); ... }
So the Array size is defined by the dt-bindings.
What you propose is a major change to the clock framework, so I would hope that Eric (the original author of this clock-driver) would address it.
Maybe someone has a better idea for a pattern to use to achieve the required while maintaining “synchronization” between defines inside the dt-binding and the driver.
On Sun, Jan 10, 2016 at 01:17:17PM +0100, Martin Sperl wrote:
Presumably just making the code not rely on having a define for the number of clocks would deal with the problem (eg, using ARRAY_SIZE internally).
ARRAY_SIZE would work fine, but the code is:
#include <dt-bindings/clock/bcm2835.h> ... struct bcm2835_cprman { struct device *dev; void __iomem *regs; spinlock_t regs_lock; const char *osc_name;
struct clk_onecell_data onecell; struct clk *clks[BCM2835_CLOCK_COUNT]; }; ... static int bcm2835_clk_probe(struct platform_device *pdev) { ... clks[BCM2835_PLLA_CORE] = bcm2835_register_pll_divider(cprman, &bcm2835_plla_core_data); ... clks[BCM2835_CLOCK_PCM] = bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data); ... }
So the Array size is defined by the dt-bindings.
What you propose is a major change to the clock framework, so I would hope that Eric (the original author of this clock-driver) would address it.
Maybe someone has a better idea for a pattern to use to achieve the required while maintaining “synchronization” between defines inside the dt-binding and the driver.
Why not just get rid of BCM2835_CLOCK_COUNT and use an internal clock count ? Something like the patch attached at the end of the mail. This has the downside to be more careful when adding a new clock.
If it is not ok, I could try to modify the clk driver to use Mark's solution if you want.
Regards,
On Sun, Jan 10, 2016 at 1:17 PM, Martin Sperl kernel@martin.sperl.org wrote:
On 10.01.2016, at 12:58, Mark Brown broonie@kernel.org wrote:
On Sun, Jan 10, 2016 at 11:55:48AM +0100, Martin Sperl wrote:
So if someone with a better idea how to keep those dt-binding includes synchronized with the definitions used by the code step forward and propose a better solution how to get that implemented?
I guess there will be a few more occurrences of clocks that are currently not defined, which will need to get introduced in the future PWM and PCM were just the last in this series.
Presumably just making the code not rely on having a define for the number of clocks would deal with the problem (eg, using ARRAY_SIZE internally).
ARRAY_SIZE would work fine, but the code is:
#include <dt-bindings/clock/bcm2835.h> ... struct bcm2835_cprman { struct device *dev; void __iomem *regs; spinlock_t regs_lock; const char *osc_name;
struct clk_onecell_data onecell; struct clk *clks[BCM2835_CLOCK_COUNT];}; ... static int bcm2835_clk_probe(struct platform_device *pdev) { ... clks[BCM2835_PLLA_CORE] = bcm2835_register_pll_divider(cprman, &bcm2835_plla_core_data); ... clks[BCM2835_CLOCK_PCM] = bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data); ... }
So the Array size is defined by the dt-bindings.
I wrote: | Hence it can better be replaced (it seems to be unused in dts files, but you | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. | This requires changing the driver to e.g. initialize clks[] in |bcm2835_clk_probe() based on a table instead of explicit code.
If you fill in clks[] from a static table, you can take ARRAY_SIZE of the static table.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10.01.2016, at 14:02, Geert Uytterhoeven geert@linux-m68k.org wrote:
I wrote: | Hence it can better be replaced (it seems to be unused in dts files, but you | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. | This requires changing the driver to e.g. initialize clks[] in |bcm2835_clk_probe() based on a table instead of explicit code.
If you fill in clks[] from a static table, you can take ARRAY_SIZE of the static table.
You mean something like the below? (note: copy/paste from console issues - spaces instead of tabs)
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index dee67b3..5ce5e7f 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -294,7 +294,7 @@ struct bcm2835_cprman { const char *osc_name;
struct clk_onecell_data onecell; - struct clk *clks[BCM2835_CLOCK_COUNT]; + struct clk *clks[]; };
static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = { .max_fb_rate = BCM2835_MAX_FB_RATE, };
+static const struct bcm2835_pll_data *bcm2835_plls[] = { + [BCM2835_PLLA] = &bcm2835_plla_data, + [BCM2835_PLLB] = &bcm2835_pllb_data, + [BCM2835_PLLC] = &bcm2835_pllc_data, + [BCM2835_PLLD] = &bcm2835_plld_data, + [BCM2835_PLLH] = &bcm2835_pllh_data, +}; + struct bcm2835_pll_divider_data { const char *name; const struct bcm2835_pll_data *source_pll; @@ -625,6 +633,20 @@ static const struct bcm2835_pll_divider_data bcm2835_pllh_p .fixed_divider = 10, };
+static const struct bcm2835_pll_divider_data *bcm2835_pll_divs[] = { + [BCM2835_PLLA_CORE] = &bcm2835_plla_core_data, + [BCM2835_PLLA_PER] = &bcm2835_plla_per_data, + [BCM2835_PLLC_CORE0] = &bcm2835_pllc_core0_data, + [BCM2835_PLLC_CORE1] = &bcm2835_pllc_core1_data, + [BCM2835_PLLC_CORE2] = &bcm2835_pllc_core2_data, + [BCM2835_PLLC_PER] = &bcm2835_pllc_per_data, + [BCM2835_PLLD_CORE] = &bcm2835_plld_core_data, + [BCM2835_PLLD_PER] = &bcm2835_plld_per_data, + [BCM2835_PLLH_RCAL] = &bcm2835_pllh_rcal_data, + [BCM2835_PLLH_AUX] = &bcm2835_pllh_aux_data, + [BCM2835_PLLH_PIX] = &bcm2835_pllh_pix_data, +}; + struct bcm2835_clock_data { const char *name;
@@ -837,6 +859,24 @@ static const struct bcm2835_clock_data bcm2835_clock_pcm_da .mash = 1, };
+static const struct bcm2835_clock_data *bcm2835_clks[] = { + [BCM2835_CLOCK_TIMER] = &bcm2835_clock_timer_data, + [BCM2835_CLOCK_OTP] = &bcm2835_clock_otp_data, + [BCM2835_CLOCK_TSENS] = &bcm2835_clock_tsens_data, + [BCM2835_CLOCK_VPU] = &bcm2835_clock_vpu_data, + [BCM2835_CLOCK_V3D] = &bcm2835_clock_v3d_data, + [BCM2835_CLOCK_ISP] = &bcm2835_clock_isp_data, + [BCM2835_CLOCK_H264] = &bcm2835_clock_h264_data, + [BCM2835_CLOCK_V3D] = &bcm2835_clock_v3d_data, + [BCM2835_CLOCK_SDRAM] = &bcm2835_clock_sdram_data, + [BCM2835_CLOCK_UART] = &bcm2835_clock_uart_data, + [BCM2835_CLOCK_VEC] = &bcm2835_clock_vec_data, + [BCM2835_CLOCK_HSM] = &bcm2835_clock_hsm_data, + [BCM2835_CLOCK_EMMC] = &bcm2835_clock_emmc_data, + [BCM2835_CLOCK_PWM] = &bcm2835_clock_pwm_data, + [BCM2835_CLOCK_PCM] = &bcm2835_clock_pcm_data, +}; + struct bcm2835_pll { struct clk_hw hw; struct bcm2835_cprman *cprman; @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev) struct clk **clks; struct bcm2835_cprman *cprman; struct resource *res; + const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls), + max(ARRAY_SIZE(bcm2835_pll_divs), + ARRAY_SIZE(bcm2835_clks))) + 1; + size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks); + size_t i;
- cprman = devm_kzalloc(dev, sizeof(*cprman), GFP_KERNEL); + cprman = devm_kzalloc(dev, alloc , GFP_KERNEL); if (!cprman) return -ENOMEM;
@@ -1578,67 +1623,43 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, cprman);
- cprman->onecell.clk_num = BCM2835_CLOCK_COUNT; + cprman->onecell.clk_num = clks_cnt; cprman->onecell.clks = cprman->clks; clks = cprman->clks;
- clks[BCM2835_PLLA] = bcm2835_register_pll(cprman, &bcm2835_plla_data); - clks[BCM2835_PLLB] = bcm2835_register_pll(cprman, &bcm2835_pllb_data); - clks[BCM2835_PLLC] = bcm2835_register_pll(cprman, &bcm2835_pllc_data); - clks[BCM2835_PLLD] = bcm2835_register_pll(cprman, &bcm2835_plld_data); - clks[BCM2835_PLLH] = bcm2835_register_pll(cprman, &bcm2835_pllh_data); - - clks[BCM2835_PLLA_CORE] = - bcm2835_register_pll_divider(cprman, &bcm2835_plla_core_data); - clks[BCM2835_PLLA_PER] = - bcm2835_register_pll_divider(cprman, &bcm2835_plla_per_data); - clks[BCM2835_PLLC_CORE0] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core0_data); - clks[BCM2835_PLLC_CORE1] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core1_data); - clks[BCM2835_PLLC_CORE2] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core2_data); - clks[BCM2835_PLLC_PER] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllc_per_data); - clks[BCM2835_PLLD_CORE] = - bcm2835_register_pll_divider(cprman, &bcm2835_plld_core_data); - clks[BCM2835_PLLD_PER] = - bcm2835_register_pll_divider(cprman, &bcm2835_plld_per_data); - clks[BCM2835_PLLH_RCAL] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllh_rcal_data); - clks[BCM2835_PLLH_AUX] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllh_aux_data); - clks[BCM2835_PLLH_PIX] = - bcm2835_register_pll_divider(cprman, &bcm2835_pllh_pix_data); - - clks[BCM2835_CLOCK_TIMER] = - bcm2835_register_clock(cprman, &bcm2835_clock_timer_data); - clks[BCM2835_CLOCK_OTP] = - bcm2835_register_clock(cprman, &bcm2835_clock_otp_data); - clks[BCM2835_CLOCK_TSENS] = - bcm2835_register_clock(cprman, &bcm2835_clock_tsens_data); - clks[BCM2835_CLOCK_VPU] = - bcm2835_register_clock(cprman, &bcm2835_clock_vpu_data); - clks[BCM2835_CLOCK_V3D] = - bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data); - clks[BCM2835_CLOCK_ISP] = - bcm2835_register_clock(cprman, &bcm2835_clock_isp_data); - clks[BCM2835_CLOCK_H264] = - bcm2835_register_clock(cprman, &bcm2835_clock_h264_data); - clks[BCM2835_CLOCK_V3D] = - bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data); - clks[BCM2835_CLOCK_SDRAM] = - bcm2835_register_clock(cprman, &bcm2835_clock_sdram_data); - clks[BCM2835_CLOCK_UART] = - bcm2835_register_clock(cprman, &bcm2835_clock_uart_data); - clks[BCM2835_CLOCK_VEC] = - bcm2835_register_clock(cprman, &bcm2835_clock_vec_data); - clks[BCM2835_CLOCK_HSM] = - bcm2835_register_clock(cprman, &bcm2835_clock_hsm_data); - clks[BCM2835_CLOCK_EMMC] = - bcm2835_register_clock(cprman, &bcm2835_clock_emmc_data); - clks[BCM2835_CLOCK_PCM] = - bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data); + /* register pll */ + for(i = 0; i< ARRAY_SIZE(bcm2835_plls); i++) { + if (!bcm2835_plls[i]) + continue; + clks[i] = bcm2835_register_pll( cprman, bcm2835_plls[i]); + } + + /* register pll divider */ + for(i = 0; i< ARRAY_SIZE(bcm2835_pll_divs); i++) { + if (!bcm2835_pll_divs[i]) + continue; + if (clks[i]) { + dev_err(dev, + "Could not register pll_div_id %i - is already defined as: %pC\n", + i, clks[i]); + continue; + } + clks[i] = bcm2835_register_pll_divider(cprman, + bcm2835_pll_divs[i]); + } + + /* register clocks */ + for(i = 0; i< ARRAY_SIZE(bcm2835_clks); i++) { + if (!bcm2835_clks[i]) + continue; + if (clks[i]) { + dev_err(dev, + "Could not register clock_id %i - is already defined as: %pC\n", + i, clks[i]); + continue; + } + clks[i] = bcm2835_register_clock(cprman, bcm2835_clks[i]); + }
/* * CM_PERIICTL (and CM_PERIACTL, CM_SYSCTL and CM_VPUCTL if @@ -1652,8 +1673,6 @@ static int bcm2835_clk_probe(struct platform_device *pdev) cprman->regs + CM_PERIICTL, CM_GATE_BIT, 0, &cprman->regs_lock);
- clks[BCM2835_CLOCK_PWM] = - bcm2835_register_clock(cprman, &bcm2835_clock_pwm_data);
return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get, &cprman->onecell);
If so, then I will send it as a patch plus the PCM patch after I have finished testing the patch.
This patch-set would also include the complete list of clocks still missing from dt-bindings.h as well as the patch that adds all the clock registers, which I had sent earlier.
Hi Martin,
On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl kernel@martin.sperl.org wrote:
On 10.01.2016, at 14:02, Geert Uytterhoeven geert@linux-m68k.org wrote: I wrote: | Hence it can better be replaced (it seems to be unused in dts files, but you | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. | This requires changing the driver to e.g. initialize clks[] in |bcm2835_clk_probe() based on a table instead of explicit code.
If you fill in clks[] from a static table, you can take ARRAY_SIZE of the static table.
You mean something like the below? (note: copy/paste from console issues - spaces instead of tabs)
More or less.
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index dee67b3..5ce5e7f 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -294,7 +294,7 @@ struct bcm2835_cprman { const char *osc_name;
struct clk_onecell_data onecell;
struct clk *clks[BCM2835_CLOCK_COUNT];
struct clk *clks[];
If all clocks would be in a single array, you could get rid of the extra dynamic allocation, and still use
struct clk *clks[ARRAY_SIZE(all_clocks_array)];
here.
};
static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = { .max_fb_rate = BCM2835_MAX_FB_RATE, };
+static const struct bcm2835_pll_data *bcm2835_plls[] = {
[BCM2835_PLLA] = &bcm2835_plla_data,[BCM2835_PLLB] = &bcm2835_pllb_data,[BCM2835_PLLC] = &bcm2835_pllc_data,[BCM2835_PLLD] = &bcm2835_plld_data,[BCM2835_PLLH] = &bcm2835_pllh_data,+};
This is a sparse array?
struct bcm2835_pll_divider_data { const char *name; const struct bcm2835_pll_data *source_pll; @@ -625,6 +633,20 @@ static const struct bcm2835_pll_divider_data bcm2835_pllh_p .fixed_divider = 10, };
+static const struct bcm2835_pll_divider_data *bcm2835_pll_divs[] = {
[BCM2835_PLLA_CORE] = &bcm2835_plla_core_data,[BCM2835_PLLA_PER] = &bcm2835_plla_per_data,[BCM2835_PLLC_CORE0] = &bcm2835_pllc_core0_data,[BCM2835_PLLC_CORE1] = &bcm2835_pllc_core1_data,[BCM2835_PLLC_CORE2] = &bcm2835_pllc_core2_data,[BCM2835_PLLC_PER] = &bcm2835_pllc_per_data,[BCM2835_PLLD_CORE] = &bcm2835_plld_core_data,[BCM2835_PLLD_PER] = &bcm2835_plld_per_data,[BCM2835_PLLH_RCAL] = &bcm2835_pllh_rcal_data,[BCM2835_PLLH_AUX] = &bcm2835_pllh_aux_data,[BCM2835_PLLH_PIX] = &bcm2835_pllh_pix_data,+};
Likewise.
struct bcm2835_clock_data { const char *name;
@@ -837,6 +859,24 @@ static const struct bcm2835_clock_data bcm2835_clock_pcm_da .mash = 1, };
+static const struct bcm2835_clock_data *bcm2835_clks[] = {
[BCM2835_CLOCK_TIMER] = &bcm2835_clock_timer_data,[BCM2835_CLOCK_OTP] = &bcm2835_clock_otp_data,[BCM2835_CLOCK_TSENS] = &bcm2835_clock_tsens_data,[BCM2835_CLOCK_VPU] = &bcm2835_clock_vpu_data,[BCM2835_CLOCK_V3D] = &bcm2835_clock_v3d_data,[BCM2835_CLOCK_ISP] = &bcm2835_clock_isp_data,[BCM2835_CLOCK_H264] = &bcm2835_clock_h264_data,[BCM2835_CLOCK_V3D] = &bcm2835_clock_v3d_data,[BCM2835_CLOCK_SDRAM] = &bcm2835_clock_sdram_data,[BCM2835_CLOCK_UART] = &bcm2835_clock_uart_data,[BCM2835_CLOCK_VEC] = &bcm2835_clock_vec_data,[BCM2835_CLOCK_HSM] = &bcm2835_clock_hsm_data,[BCM2835_CLOCK_EMMC] = &bcm2835_clock_emmc_data,[BCM2835_CLOCK_PWM] = &bcm2835_clock_pwm_data,[BCM2835_CLOCK_PCM] = &bcm2835_clock_pcm_data,+};
Likewise.
struct bcm2835_pll { struct clk_hw hw; struct bcm2835_cprman *cprman; @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev) struct clk **clks; struct bcm2835_cprman *cprman; struct resource *res;
const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),max(ARRAY_SIZE(bcm2835_pll_divs),ARRAY_SIZE(bcm2835_clks))) + 1;size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);size_t i;
If you combine all 3 arrays in a single non-sparse array, you could get rid of the dynamic allocation using the maximum of the 3 sizes, and can just use a single ARRAY_SIZE().
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10.01.2016, at 19:56, Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Martin,
On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl kernel@martin.sperl.org wrote:
On 10.01.2016, at 14:02, Geert Uytterhoeven geert@linux-m68k.org wrote: I wrote: | Hence it can better be replaced (it seems to be unused in dts files, but you | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. | This requires changing the driver to e.g. initialize clks[] in |bcm2835_clk_probe() based on a table instead of explicit code.
If you fill in clks[] from a static table, you can take ARRAY_SIZE of the static table.
You mean something like the below? (note: copy/paste from console issues - spaces instead of tabs)
More or less.
};
static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = { .max_fb_rate = BCM2835_MAX_FB_RATE, };
+static const struct bcm2835_pll_data *bcm2835_plls[] = {
[BCM2835_PLLA] = &bcm2835_plla_data,[BCM2835_PLLB] = &bcm2835_pllb_data,[BCM2835_PLLC] = &bcm2835_pllc_data,[BCM2835_PLLD] = &bcm2835_plld_data,[BCM2835_PLLH] = &bcm2835_pllh_data,+};
This is a sparse array?
Yes - for all.
struct bcm2835_pll { struct clk_hw hw; struct bcm2835_cprman *cprman; @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev) struct clk **clks; struct bcm2835_cprman *cprman; struct resource *res;
const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),max(ARRAY_SIZE(bcm2835_pll_divs),ARRAY_SIZE(bcm2835_clks))) + 1;size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);size_t i;If you combine all 3 arrays in a single non-sparse array, you could get rid of the dynamic allocation using the maximum of the 3 sizes, and can just use a single ARRAY_SIZE().
The problem is that we need different initialization methods for pll, pll-dividers and derived clocks, so we can not easily make them a common setting unless we would use function and void pointers to work arround this, which would make it less readable (and more code again just for the - repeated - assignment).
As far as I can tell from my testing the patched version works and the kernel boots correctly.
I will respin the patchset tomorrow - it might take slightly longer, as I found that the fractional clock divider is not working unless the MASH functionality is activated in the registers. This results in “frequency shifts” for audio, which we want to avoid - this means we need another patch to enable this MASH feature.
Hi Martin,
On Sun, Jan 10, 2016 at 8:07 PM, Martin Sperl kernel@martin.sperl.org wrote:
On 10.01.2016, at 19:56, Geert Uytterhoeven geert@linux-m68k.org wrote: On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl kernel@martin.sperl.org wrote:
On 10.01.2016, at 14:02, Geert Uytterhoeven geert@linux-m68k.org wrote: I wrote: | Hence it can better be replaced (it seems to be unused in dts files, but you | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver. | This requires changing the driver to e.g. initialize clks[] in |bcm2835_clk_probe() based on a table instead of explicit code.
If you fill in clks[] from a static table, you can take ARRAY_SIZE of the static table.
You mean something like the below? (note: copy/paste from console issues - spaces instead of tabs)
More or less.
};
static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = { .max_fb_rate = BCM2835_MAX_FB_RATE, };
+static const struct bcm2835_pll_data *bcm2835_plls[] = {
[BCM2835_PLLA] = &bcm2835_plla_data,[BCM2835_PLLB] = &bcm2835_pllb_data,[BCM2835_PLLC] = &bcm2835_pllc_data,[BCM2835_PLLD] = &bcm2835_plld_data,[BCM2835_PLLH] = &bcm2835_pllh_data,+};
This is a sparse array?
Yes - for all.
struct bcm2835_pll { struct clk_hw hw; struct bcm2835_cprman *cprman; @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev) struct clk **clks; struct bcm2835_cprman *cprman; struct resource *res;
const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),max(ARRAY_SIZE(bcm2835_pll_divs),ARRAY_SIZE(bcm2835_clks))) + 1;size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);size_t i;If you combine all 3 arrays in a single non-sparse array, you could get rid of the dynamic allocation using the maximum of the 3 sizes, and can just use a single ARRAY_SIZE().
The problem is that we need different initialization methods for pll, pll-dividers and derived clocks, so we can not easily make them a common setting unless we would use function and void pointers to work arround this, which would make it less readable (and more code again just for the - repeated - assignment).
You can store the clock type in the array elements, too?
I'm doing something similar, cfr. drivers/clk/shmobile/*-cpg-mssr.* in next.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Sunday 10 January 2016 13:17:17 Martin Sperl wrote:
What you propose is a major change to the clock framework, so I would hope that Eric (the original author of this clock-driver) would address it.
Maybe someone has a better idea for a pattern to use to achieve the required while maintaining “synchronization” between defines inside the dt-binding and the driver.
It's funny to look at the register list:
#define CM_VPUCTL 0x008 #define CM_VPUDIV 0x00c #define CM_SYSCTL 0x010 #define CM_SYSDIV 0x014 #define CM_PERIACTL 0x018 #define CM_PERIADIV 0x01c #define CM_PERIICTL 0x020 #define CM_PERIIDIV 0x024 #define CM_H264CTL 0x028 #define CM_H264DIV 0x02c
This one seems completely regular, there is always a pair of CTL and DIV registers, so I would have guessed that we could just take the index of that to completely avoid the need for the header file and just have a lookup table of each index.
I can see two possible ways forward:
a) deprecate the existing binding and write a new simpler driver to replace it with one that does not need the header. We probably need to keep both drivers around indefinitely though to deal with people that have their own dtb files, so this is a bit awkward.
b) look at all the holes in the table and define new numbers for them now, then remove the count as the driver can simply hardcode the maximum number as it knows it will never change.
Arnd
On 11.01.2016, at 14:38, Arnd Bergmann arnd@arndb.de wrote:
On Sunday 10 January 2016 13:17:17 Martin Sperl wrote:
What you propose is a major change to the clock framework, so I would hope that Eric (the original author of this clock-driver) would address it.
Maybe someone has a better idea for a pattern to use to achieve the required while maintaining “synchronization” between defines inside the dt-binding and the driver.
It's funny to look at the register list:
#define CM_VPUCTL 0x008 #define CM_VPUDIV 0x00c #define CM_SYSCTL 0x010 #define CM_SYSDIV 0x014 #define CM_PERIACTL 0x018 #define CM_PERIADIV 0x01c #define CM_PERIICTL 0x020 #define CM_PERIIDIV 0x024 #define CM_H264CTL 0x028 #define CM_H264DIV 0x02c
This one seems completely regular, there is always a pair of CTL and DIV registers, so I would have guessed that we could just take the index of that to completely avoid the need for the header file and just have a lookup table of each index.
I can see two possible ways forward:
a) deprecate the existing binding and write a new simpler driver to replace it with one that does not need the header. We probably need to keep both drivers around indefinitely though to deal with people that have their own dtb files, so this is a bit awkward.
I was thinking about this as well, but am concerned about the dt-changes.
Also initialization order may be an issue, so I have avoided that.
b) look at all the holes in the table and define new numbers for them now, then remove the count as the driver can simply hardcode the maximum number as it knows it will never change.
There are clocks for most of them - See my patch I sent some time ago.
Right now I have got a working version with the following patches: 817176d clk: bcm2835: add missing clocks 8f74701 clk: bcm2835: enable management of PCM clock 62a7d94 clk: bcm2835: move to a different initialization scheme.
There is one more thing that I would like to do before submitting those as a patch-set: enable the fractual divider, because right now we calculate the fractual divider value correctly, but we do not set the flag to enable it, so we are actually running too fast most of the times...
From: Martin Sperl kernel@martin.sperl.org
Since the move to the new clock framework with commit 94cb7f76caa0b337 ("Switch to using the new clock driver support") this driver was no longer functional as it was manipulating the clock registers locally without going true the framework.
This patch moves to use the new clock framework and also moves away from the hardcoded address offsets for DMA getting the dma-address directly from the device tree.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- sound/soc/bcm/bcm2835-i2s.c | 281 ++++++++++--------------------------------- 1 file changed, 63 insertions(+), 218 deletions(-)
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index 3303d5f..794ada5 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -37,6 +37,7 @@ #include <linux/init.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/of_address.h> #include <linux/slab.h>
#include <sound/core.h> @@ -46,55 +47,6 @@ #include <sound/pcm_params.h> #include <sound/soc.h>
-/* Clock registers */ -#define BCM2835_CLK_PCMCTL_REG 0x00 -#define BCM2835_CLK_PCMDIV_REG 0x04 - -/* Clock register settings */ -#define BCM2835_CLK_PASSWD (0x5a000000) -#define BCM2835_CLK_PASSWD_MASK (0xff000000) -#define BCM2835_CLK_MASH(v) ((v) << 9) -#define BCM2835_CLK_FLIP BIT(8) -#define BCM2835_CLK_BUSY BIT(7) -#define BCM2835_CLK_KILL BIT(5) -#define BCM2835_CLK_ENAB BIT(4) -#define BCM2835_CLK_SRC(v) (v) - -#define BCM2835_CLK_SHIFT (12) -#define BCM2835_CLK_DIVI(v) ((v) << BCM2835_CLK_SHIFT) -#define BCM2835_CLK_DIVF(v) (v) -#define BCM2835_CLK_DIVF_MASK (0xFFF) - -enum { - BCM2835_CLK_MASH_0 = 0, - BCM2835_CLK_MASH_1, - BCM2835_CLK_MASH_2, - BCM2835_CLK_MASH_3, -}; - -enum { - BCM2835_CLK_SRC_GND = 0, - BCM2835_CLK_SRC_OSC, - BCM2835_CLK_SRC_DBG0, - BCM2835_CLK_SRC_DBG1, - BCM2835_CLK_SRC_PLLA, - BCM2835_CLK_SRC_PLLC, - BCM2835_CLK_SRC_PLLD, - BCM2835_CLK_SRC_HDMI, -}; - -/* Most clocks are not useable (freq = 0) */ -static const unsigned int bcm2835_clk_freq[BCM2835_CLK_SRC_HDMI+1] = { - [BCM2835_CLK_SRC_GND] = 0, - [BCM2835_CLK_SRC_OSC] = 19200000, - [BCM2835_CLK_SRC_DBG0] = 0, - [BCM2835_CLK_SRC_DBG1] = 0, - [BCM2835_CLK_SRC_PLLA] = 0, - [BCM2835_CLK_SRC_PLLC] = 0, - [BCM2835_CLK_SRC_PLLD] = 500000000, - [BCM2835_CLK_SRC_HDMI] = 0, -}; - /* I2S registers */ #define BCM2835_I2S_CS_A_REG 0x00 #define BCM2835_I2S_FIFO_A_REG 0x04 @@ -158,10 +110,6 @@ static const unsigned int bcm2835_clk_freq[BCM2835_CLK_SRC_HDMI+1] = { #define BCM2835_I2S_INT_RXR BIT(1) #define BCM2835_I2S_INT_TXW BIT(0)
-/* I2S DMA interface */ -/* FIXME: Needs IOMMU support */ -#define BCM2835_VCMMU_SHIFT (0x7E000000 - 0x20000000) - /* General device struct */ struct bcm2835_i2s_dev { struct device *dev; @@ -169,21 +117,23 @@ struct bcm2835_i2s_dev { unsigned int fmt; unsigned int bclk_ratio;
- struct regmap *i2s_regmap; - struct regmap *clk_regmap; + struct regmap *i2s_regmap; + struct clk *clk; + bool clk_prepared; };
static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev) { - /* Start the clock if in master mode */ unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
+ if (dev->clk_prepared) + return; + switch (master) { case SND_SOC_DAIFMT_CBS_CFS: case SND_SOC_DAIFMT_CBS_CFM: - regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, - BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB, - BCM2835_CLK_PASSWD | BCM2835_CLK_ENAB); + clk_prepare_enable(dev->clk); + dev->clk_prepared = true; break; default: break; @@ -192,28 +142,9 @@ static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev)
static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev) { - uint32_t clkreg; - int timeout = 1000; - - /* Stop clock */ - regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, - BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB, - BCM2835_CLK_PASSWD); - - /* Wait for the BUSY flag going down */ - while (--timeout) { - regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg); - if (!(clkreg & BCM2835_CLK_BUSY)) - break; - } - - if (!timeout) { - /* KILL the clock */ - dev_err(dev->dev, "I2S clock didn't stop. Kill the clock!\n"); - regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, - BCM2835_CLK_KILL | BCM2835_CLK_PASSWD_MASK, - BCM2835_CLK_KILL | BCM2835_CLK_PASSWD); - } + if (dev->clk_prepared) + clk_disable_unprepare(dev->clk); + dev->clk_prepared = false; }
static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev, @@ -223,8 +154,7 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev, uint32_t syncval; uint32_t csreg; uint32_t i2s_active_state; - uint32_t clkreg; - uint32_t clk_active_state; + bool clk_was_prepared; uint32_t off; uint32_t clr;
@@ -238,15 +168,10 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev, regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg); i2s_active_state = csreg & (BCM2835_I2S_RXON | BCM2835_I2S_TXON);
- regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg); - clk_active_state = clkreg & BCM2835_CLK_ENAB; - /* Start clock if not running */ - if (!clk_active_state) { - regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, - BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB, - BCM2835_CLK_PASSWD | BCM2835_CLK_ENAB); - } + clk_was_prepared = dev->clk_prepared; + if (!clk_was_prepared) + bcm2835_i2s_start_clock(dev);
/* Stop I2S module */ regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0); @@ -280,7 +205,7 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev, dev_err(dev->dev, "I2S SYNC error!\n");
/* Stop clock if it was not running before */ - if (!clk_active_state) + if (!clk_was_prepared) bcm2835_i2s_stop_clock(dev);
/* Restore I2S state */ @@ -309,19 +234,9 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai); - unsigned int sampling_rate = params_rate(params); unsigned int data_length, data_delay, bclk_ratio; unsigned int ch1pos, ch2pos, mode, format; - unsigned int mash = BCM2835_CLK_MASH_1; - unsigned int divi, divf, target_frequency; - int clk_src = -1; - unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK; - bool bit_master = (master == SND_SOC_DAIFMT_CBS_CFS - || master == SND_SOC_DAIFMT_CBS_CFM); - - bool frame_master = (master == SND_SOC_DAIFMT_CBS_CFS - || master == SND_SOC_DAIFMT_CBM_CFS); uint32_t csreg;
/* @@ -356,69 +271,11 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, /* If bclk_ratio already set, use that one. */ if (dev->bclk_ratio) bclk_ratio = dev->bclk_ratio; + else + bclk_ratio = 2 * data_length;
- /* - * Clock Settings - * - * The target frequency of the bit clock is - * sampling rate * frame length - * - * Integer mode: - * Sampling rates that are multiples of 8000 kHz - * can be driven by the oscillator of 19.2 MHz - * with an integer divider as long as the frame length - * is an integer divider of 19200000/8000=2400 as set up above. - * This is no longer possible if the sampling rate - * is too high (e.g. 192 kHz), because the oscillator is too slow. - * - * MASH mode: - * For all other sampling rates, it is not possible to - * have an integer divider. Approximate the clock - * with the MASH module that induces a slight frequency - * variance. To minimize that it is best to have the fastest - * clock here. That is PLLD with 500 MHz. - */ - target_frequency = sampling_rate * bclk_ratio; - clk_src = BCM2835_CLK_SRC_OSC; - mash = BCM2835_CLK_MASH_0; - - if (bcm2835_clk_freq[clk_src] % target_frequency == 0 - && bit_master && frame_master) { - divi = bcm2835_clk_freq[clk_src] / target_frequency; - divf = 0; - } else { - uint64_t dividend; - - if (!dev->bclk_ratio) { - /* - * Overwrite bclk_ratio, because the - * above trick is not needed or can - * not be used. - */ - bclk_ratio = 2 * data_length; - } - - target_frequency = sampling_rate * bclk_ratio; - - clk_src = BCM2835_CLK_SRC_PLLD; - mash = BCM2835_CLK_MASH_1; - - dividend = bcm2835_clk_freq[clk_src]; - dividend <<= BCM2835_CLK_SHIFT; - do_div(dividend, target_frequency); - divi = dividend >> BCM2835_CLK_SHIFT; - divf = dividend & BCM2835_CLK_DIVF_MASK; - } - - /* Set clock divider */ - regmap_write(dev->clk_regmap, BCM2835_CLK_PCMDIV_REG, BCM2835_CLK_PASSWD - | BCM2835_CLK_DIVI(divi) - | BCM2835_CLK_DIVF(divf)); - - /* Setup clock, but don't start it yet */ - regmap_write(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, BCM2835_CLK_PASSWD - | BCM2835_CLK_MASH(mash) - | BCM2835_CLK_SRC(clk_src)); + /* set target clock rate*/ + clk_set_rate(dev->clk, sampling_rate * bclk_ratio);
/* Setup the frame format */ format = BCM2835_I2S_CHEN; @@ -692,7 +549,7 @@ static const struct snd_soc_dai_ops bcm2835_i2s_dai_ops = { .trigger = bcm2835_i2s_trigger, .hw_params = bcm2835_i2s_hw_params, .set_fmt = bcm2835_i2s_set_dai_fmt, - .set_bclk_ratio = bcm2835_i2s_set_dai_bclk_ratio + .set_bclk_ratio = bcm2835_i2s_set_dai_bclk_ratio, };
static int bcm2835_i2s_dai_probe(struct snd_soc_dai *dai) @@ -750,34 +607,14 @@ static bool bcm2835_i2s_precious_reg(struct device *dev, unsigned int reg) }; }
-static bool bcm2835_clk_volatile_reg(struct device *dev, unsigned int reg) -{ - switch (reg) { - case BCM2835_CLK_PCMCTL_REG: - return true; - default: - return false; - }; -} - -static const struct regmap_config bcm2835_regmap_config[] = { - { - .reg_bits = 32, - .reg_stride = 4, - .val_bits = 32, - .max_register = BCM2835_I2S_GRAY_REG, - .precious_reg = bcm2835_i2s_precious_reg, - .volatile_reg = bcm2835_i2s_volatile_reg, - .cache_type = REGCACHE_RBTREE, - }, - { - .reg_bits = 32, - .reg_stride = 4, - .val_bits = 32, - .max_register = BCM2835_CLK_PCMDIV_REG, - .volatile_reg = bcm2835_clk_volatile_reg, - .cache_type = REGCACHE_RBTREE, - }, +static const struct regmap_config bcm2835_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = BCM2835_I2S_GRAY_REG, + .precious_reg = bcm2835_i2s_precious_reg, + .volatile_reg = bcm2835_i2s_volatile_reg, + .cache_type = REGCACHE_RBTREE, };
static const struct snd_soc_component_driver bcm2835_i2s_component = { @@ -787,42 +624,50 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = { static int bcm2835_i2s_probe(struct platform_device *pdev) { struct bcm2835_i2s_dev *dev; - int i; int ret; - struct regmap *regmap[2]; - struct resource *mem[2]; - - /* Request both ioareas */ - for (i = 0; i <= 1; i++) { - void __iomem *base; - - mem[i] = platform_get_resource(pdev, IORESOURCE_MEM, i); - base = devm_ioremap_resource(&pdev->dev, mem[i]); - if (IS_ERR(base)) - return PTR_ERR(base); - - regmap[i] = devm_regmap_init_mmio(&pdev->dev, base, - &bcm2835_regmap_config[i]); - if (IS_ERR(regmap[i])) - return PTR_ERR(regmap[i]); - } + struct resource *mem; + void __iomem *base; + const __be32 *addr; + dma_addr_t dma_base;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); if (!dev) return -ENOMEM;
- dev->i2s_regmap = regmap[0]; - dev->clk_regmap = regmap[1]; + /* get the clock */ + dev->clk_prepared = false; + dev->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(dev->clk)) { + dev_err(&pdev->dev, "could not get clk: %ld\n", + PTR_ERR(dev->clk)); + return PTR_ERR(dev->clk); + } + + /* Request ioarea */ + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(&pdev->dev, mem); + if (IS_ERR(base)) + return PTR_ERR(base); + + dev->i2s_regmap = devm_regmap_init_mmio(&pdev->dev, base, + &bcm2835_regmap_config); + if (IS_ERR(dev->i2s_regmap)) + return PTR_ERR(dev->i2s_regmap); + + /* Set the DMA address - we have to parse DT ourselves */ + addr = of_get_address(pdev->dev.of_node, 0, NULL, NULL); + if (!addr) { + dev_err(&pdev->dev, "could not get DMA-register address\n"); + return -EINVAL; + } + dma_base = be32_to_cpup(addr);
- /* Set the DMA address */ dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr = - (dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG - + BCM2835_VCMMU_SHIFT; + dma_base + BCM2835_I2S_FIFO_A_REG;
dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr = - (dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG - + BCM2835_VCMMU_SHIFT; + dma_base + BCM2835_I2S_FIFO_A_REG;
/* Set the bus width */ dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr_width =
From: Martin Sperl kernel@martin.sperl.org
Since the move to the new clock framework with commit 94cb7f76caa0b337 ("Switch to using the new clock driver support") the bcm2835-i2s driver was no longer working.
This patch fixes the address ranges: * remove the PCM clock register range that is owned by the clockmanager * fix the length, which did not include the last register of this device
It also adds the required pcm-clock with the corresponding default clock and rate.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- arch/arm/boot/dts/bcm2835.dtsi | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi index aef64de..83d9787 100644 --- a/arch/arm/boot/dts/bcm2835.dtsi +++ b/arch/arm/boot/dts/bcm2835.dtsi @@ -120,9 +120,8 @@
i2s: i2s@7e203000 { compatible = "brcm,bcm2835-i2s"; - reg = <0x7e203000 0x20>, - <0x7e101098 0x02>; - + reg = <0x7e203000 0x24>; + clocks = <&clocks BCM2835_CLOCK_PCM>; dmas = <&dma 2>, <&dma 3>; dma-names = "tx", "rx";
From: Martin Sperl kernel@martin.sperl.org
The bcm2835-i2s driver has been updated to use the new clock framework for the bcm2835 SOC.
This patch documents the required changes to the bindings.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt index 65783de..b331f26 100644 --- a/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt +++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt @@ -4,11 +4,10 @@ Required properties: - compatible: "brcm,bcm2835-i2s" - reg: A list of base address and size entries: * The first entry should cover the PCM registers - * The second entry should cover the PCM clock registers +- clocks: the (PCM) clock to use - dmas: List of DMA controller phandle and DMA request line ordered pairs. - dma-names: Identifier string for each DMA request line in the dmas property. These strings correspond 1:1 with the ordered pairs in dmas. - One of the DMA channels will be responsible for transmission (should be named "tx") and one for reception (should be named "rx").
@@ -16,8 +15,8 @@ Example:
bcm2835_i2s: i2s@7e203000 { compatible = "brcm,bcm2835-i2s"; - reg = <0x7e203000 0x20>, - <0x7e101098 0x02>; + reg = <0x7e203000 0x24>; + clocks = <&clocks BCM2835_CLOCK_PCM>;
dmas = <&dma 2>, <&dma 3>;
On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel@martin.sperl.org wrote:
From: Martin Sperl kernel@martin.sperl.org
The bcm2835-i2s driver has been updated to use the new clock framework for the bcm2835 SOC.
You are breaking compatibility with the driver change. New kernels will not work with old dtbs as you have found. What should be done is fix the driver to work with the old and new dtb.
This patch documents the required changes to the bindings.
Signed-off-by: Martin Sperl kernel@martin.sperl.org
Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
But I leave it to platform maintainers to decide if breaking compatibility is okay, so:
Acked-by: Rob Herring robh@kernel.org
On 09.01.2016, at 23:45, Rob Herring robh@kernel.org wrote:
On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel@martin.sperl.org wrote:
From: Martin Sperl kernel@martin.sperl.org
The bcm2835-i2s driver has been updated to use the new clock framework for the bcm2835 SOC.
You are breaking compatibility with the driver change. New kernels will not work with old dtbs as you have found. What should be done is fix the driver to work with the old and new dtb.
Actually the driver/device-tree is already broken with commit 94cb7f76caa0 ("ARM: bcm2835: Switch to using the new clock driver support”)
Because there we already have the following conflicting definitions:
clocks: cprman@7e101000 { compatible = "brcm,bcm2835-cprman"; #clock-cells = <1>; reg = <0x7e101000 0x2000>; /* CPRMAN derives everything from the platform's * oscillator. */ clocks = <&clk_osc>; };
i2s: i2s@7e203000 { compatible = "brcm,bcm2835-i2s"; reg = <0x7e203000 0x20>, <0x7e101098 0x02>; dmas = <&dma 2>, <&dma 3>; dma-names = "tx", "rx"; status = "disabled"; };
The i2s driver fails to load because the second reg range overlaps with the range from clocks (which was introduced with the commit mentioned above.
This patch only changes the driver to work again without this second reg address range and using the clocks property instead.
This patch documents the required changes to the bindings.
Signed-off-by: Martin Sperl kernel@martin.sperl.org
Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
But I leave it to platform maintainers to decide if breaking compatibility is okay, so:
Acked-by: Rob Herring robh@kernel.org
Thanks, Martin
participants (7)
-
Arnd Bergmann -
Geert Uytterhoeven -
kernel＠martin.sperl.org -
Mark Brown -
Martin Sperl
-
Remi Pommarel -
Rob Herring