[PATCH 0/5] ASOC: bcm2835: move bcm2835-i2s to use clock framework

[PATCH 0/5] ASOC: bcm2835: move bcm2835-i2s to use clock framework

[kernel]

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

[PATCH 0/5] ASOC: bcm2835: move bcm2835-i2s to use clock framework

[kernel at martin.sperl.org]

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

[PATCH 1/5] ASoC: bcm2835: cleanup includes by ordering them alphabetically

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Cleanup of includes so that they are ordered alphabetically.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ASoC: bcm2835: cleanup includes by ordering them alphabetically

[kernel]

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
-- 
1.7.10.4

[PATCH 1/5] ASoC: bcm2835: cleanup includes by ordering them alphabetically

[kernel at martin.sperl.org]

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
-- 
1.7.10.4

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[kernel]

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
-- 
1.7.10.4

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[kernel at martin.sperl.org]

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
-- 
1.7.10.4

[PATCH 3/5] ASoC: bcm2835: move to use the clock framework

[kernel]

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 =
-- 
1.7.10.4

[PATCH 3/5] ASoC: bcm2835: move to use the clock framework

[kernel at martin.sperl.org]

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 =
-- 
1.7.10.4

[PATCH 4/5] ARM: bcm2835: I2S: use new register-range and clock framework

[kernel]

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";
-- 
1.7.10.4

[PATCH 4/5] ARM: bcm2835: I2S: use new register-range and clock framework

[kernel at martin.sperl.org]

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 at 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";
-- 
1.7.10.4

[PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[kernel]

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>;
-- 
1.7.10.4

[PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[kernel at martin.sperl.org]

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 at 7e203000 {
 	compatible = "brcm,bcm2835-i2s";
-	reg = <0x7e203000 0x20>,
-	      <0x7e101098 0x02>;
+	reg = <0x7e203000 0x24>;
+	clocks = <&clocks BCM2835_CLOCK_PCM>;
 
 	dmas = <&dma 2>,
 	       <&dma 3>;
-- 
1.7.10.4

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

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

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

On Saturday 09 January 2016 09:25:54 kernel at 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

Re: [PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Rob Herring]

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>

[PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Rob Herring]

On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel at 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>

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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 at 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 at 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 10.01.2016, at 10:30, Geert Uytterhoeven <geert@linux-m68k.org> =
wrote:
>=20
> Hi Arnd,
>=20
> 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
>>>=20
>>> -#define BCM2835_CLOCK_COUNT            31
>>> +#define BCM2835_CLOCK_COUNT            32
>>=20
>> 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.
>=20
> 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...
>=20
> 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=

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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 at 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

Re: [PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Martin Sperl]

> On 09.01.2016, at 23:45, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks,
	Martin--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Martin Sperl]

> On 09.01.2016, at 23:45, Rob Herring <robh@kernel.org> wrote:
>=20
> On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel@martin.sperl.org =
wrote:
>> From: Martin Sperl <kernel@martin.sperl.org>
>>=20
>> The bcm2835-i2s driver has been updated to use the new clock =
framework
>> for the bcm2835 SOC.
>=20
> You are breaking compatibility with the driver change. New kernels =
will=20
> not work with old dtbs as you have found. What should be done is fix =
the=20
> 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=E2=80=9D)

Because there we already have the following conflicting definitions:

clocks: cprman@7e101000 {
	compatible =3D "brcm,bcm2835-cprman";
	#clock-cells =3D <1>;
	reg =3D <0x7e101000 0x2000>;
	/* CPRMAN derives everything from the platform's
	* oscillator.
	*/
	clocks =3D <&clk_osc>;
};

i2s: i2s@7e203000 {
	compatible =3D "brcm,bcm2835-i2s";
	reg =3D <0x7e203000 0x20>,
	      <0x7e101098 0x02>;
	dmas =3D <&dma 2>,
	       <&dma 3>;
	dma-names =3D "tx", "rx";
	status =3D "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.

>=20
>> This patch documents the required changes to the bindings.
>>=20
>> 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(-)
>=20
> But I leave it to platform maintainers to decide if breaking=20
> compatibility is okay, so:
>=20
> Acked-by: Rob Herring <robh@kernel.org>

Thanks,
	Martin=

[PATCH 5/5] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Martin Sperl]

> On 09.01.2016, at 23:45, Rob Herring <robh@kernel.org> wrote:
> 
> On Sat, Jan 09, 2016 at 09:25:57AM +0000, kernel at 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 at 7e101000 {
	compatible = "brcm,bcm2835-cprman";
	#clock-cells = <1>;
	reg = <0x7e101000 0x2000>;
	/* CPRMAN derives everything from the platform's
	* oscillator.
	*/
	clocks = <&clk_osc>;
};

i2s: i2s at 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Mark Brown]

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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160110/4fba2e82/attachment.sig>

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 10.01.2016, at 12:58, Mark Brown <broonie@kernel.org> wrote:
>=20
> On Sun, Jan 10, 2016 at 11:55:48AM +0100, Martin Sperl wrote:
>=20
>> 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?
>=20
>> 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.
>=20
> 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:=20

#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] =3D
		bcm2835_register_pll_divider(cprman, =
&bcm2835_plla_core_data);
...
        clks[BCM2835_CLOCK_PCM] =3D
                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=20
the required while maintaining =E2=80=9Csynchronization=E2=80=9D between =
defines
inside the dt-binding and the driver.

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Remi Pommarel]

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,

-- 
Remi

--------------------------------->8---------------------------------------
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 015e687..f558c5b 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -281,6 +281,8 @@
 #define LOCK_TIMEOUT_NS                100000000
 #define BCM2835_MAX_FB_RATE    1750000000u
 
+#define BCM2835_CLOCK_MAX BCM2835_CLOCK_PCM
+
 struct bcm2835_cprman {
        struct device *dev;
        void __iomem *regs;
@@ -288,7 +290,7 @@ struct bcm2835_cprman {
        const char *osc_name;
 
        struct clk_onecell_data onecell;
-       struct clk *clks[BCM2835_CLOCK_COUNT];
+       struct clk *clks[BCM2835_CLOCK_MAX + 1];
 };
 
 static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val)

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Remi Pommarel]

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,

-- 
Remi

--------------------------------->8---------------------------------------
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 015e687..f558c5b 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -281,6 +281,8 @@
 #define LOCK_TIMEOUT_NS                100000000
 #define BCM2835_MAX_FB_RATE    1750000000u
 
+#define BCM2835_CLOCK_MAX BCM2835_CLOCK_PCM
+
 struct bcm2835_cprman {
        struct device *dev;
        void __iomem *regs;
@@ -288,7 +290,7 @@ struct bcm2835_cprman {
        const char *osc_name;
 
        struct clk_onecell_data onecell;
-       struct clk *clks[BCM2835_CLOCK_COUNT];
+       struct clk *clks[BCM2835_CLOCK_MAX + 1];
 };
 
 static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val)

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 10.01.2016, at 14:02, Geert Uytterhoeven <geert@linux-m68k.org> =
wrote:
>=20
> 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.
>=20
> 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 =3D {
        .max_fb_rate =3D BCM2835_MAX_FB_RATE,
 };

+static const struct bcm2835_pll_data *bcm2835_plls[] =3D {
+       [BCM2835_PLLA] =3D &bcm2835_plla_data,
+       [BCM2835_PLLB] =3D &bcm2835_pllb_data,
+       [BCM2835_PLLC] =3D &bcm2835_pllc_data,
+       [BCM2835_PLLD] =3D &bcm2835_plld_data,
+       [BCM2835_PLLH] =3D &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 =3D 10,
 };

+static const struct bcm2835_pll_divider_data *bcm2835_pll_divs[] =3D {
+       [BCM2835_PLLA_CORE]     =3D &bcm2835_plla_core_data,
+       [BCM2835_PLLA_PER]      =3D &bcm2835_plla_per_data,
+       [BCM2835_PLLC_CORE0]    =3D &bcm2835_pllc_core0_data,
+       [BCM2835_PLLC_CORE1]    =3D &bcm2835_pllc_core1_data,
+       [BCM2835_PLLC_CORE2]    =3D &bcm2835_pllc_core2_data,
+       [BCM2835_PLLC_PER]      =3D &bcm2835_pllc_per_data,
+       [BCM2835_PLLD_CORE]     =3D &bcm2835_plld_core_data,
+       [BCM2835_PLLD_PER]      =3D &bcm2835_plld_per_data,
+       [BCM2835_PLLH_RCAL]     =3D &bcm2835_pllh_rcal_data,
+       [BCM2835_PLLH_AUX]      =3D &bcm2835_pllh_aux_data,
+       [BCM2835_PLLH_PIX]      =3D &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 =3D 1,
 };

+static const struct bcm2835_clock_data *bcm2835_clks[] =3D {
+       [BCM2835_CLOCK_TIMER]   =3D &bcm2835_clock_timer_data,
+       [BCM2835_CLOCK_OTP]     =3D &bcm2835_clock_otp_data,
+       [BCM2835_CLOCK_TSENS]   =3D &bcm2835_clock_tsens_data,
+       [BCM2835_CLOCK_VPU]     =3D &bcm2835_clock_vpu_data,
+       [BCM2835_CLOCK_V3D]     =3D &bcm2835_clock_v3d_data,
+       [BCM2835_CLOCK_ISP]     =3D &bcm2835_clock_isp_data,
+       [BCM2835_CLOCK_H264]    =3D &bcm2835_clock_h264_data,
+       [BCM2835_CLOCK_V3D]     =3D &bcm2835_clock_v3d_data,
+       [BCM2835_CLOCK_SDRAM]   =3D &bcm2835_clock_sdram_data,
+       [BCM2835_CLOCK_UART]    =3D &bcm2835_clock_uart_data,
+       [BCM2835_CLOCK_VEC]     =3D &bcm2835_clock_vec_data,
+       [BCM2835_CLOCK_HSM]     =3D &bcm2835_clock_hsm_data,
+       [BCM2835_CLOCK_EMMC]    =3D &bcm2835_clock_emmc_data,
+       [BCM2835_CLOCK_PWM]     =3D &bcm2835_clock_pwm_data,
+       [BCM2835_CLOCK_PCM]     =3D &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 =3D max(ARRAY_SIZE(bcm2835_plls),
+                                max(ARRAY_SIZE(bcm2835_pll_divs),
+                                    ARRAY_SIZE(bcm2835_clks))) + 1;
+       size_t alloc =3D sizeof(*cprman) + clks_cnt * sizeof(*clks);
+       size_t i;

-       cprman =3D devm_kzalloc(dev, sizeof(*cprman), GFP_KERNEL);
+       cprman =3D 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 =3D BCM2835_CLOCK_COUNT;
+       cprman->onecell.clk_num =3D clks_cnt;
        cprman->onecell.clks =3D cprman->clks;
        clks =3D cprman->clks;

-       clks[BCM2835_PLLA] =3D bcm2835_register_pll(cprman, =
&bcm2835_plla_data);
-       clks[BCM2835_PLLB] =3D bcm2835_register_pll(cprman, =
&bcm2835_pllb_data);
-       clks[BCM2835_PLLC] =3D bcm2835_register_pll(cprman, =
&bcm2835_pllc_data);
-       clks[BCM2835_PLLD] =3D bcm2835_register_pll(cprman, =
&bcm2835_plld_data);
-       clks[BCM2835_PLLH] =3D bcm2835_register_pll(cprman, =
&bcm2835_pllh_data);
-
-       clks[BCM2835_PLLA_CORE] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_plla_core_data);
-       clks[BCM2835_PLLA_PER] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_plla_per_data);
-       clks[BCM2835_PLLC_CORE0] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllc_core0_data);
-       clks[BCM2835_PLLC_CORE1] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllc_core1_data);
-       clks[BCM2835_PLLC_CORE2] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllc_core2_data);
-       clks[BCM2835_PLLC_PER] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllc_per_data);
-       clks[BCM2835_PLLD_CORE] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_plld_core_data);
-       clks[BCM2835_PLLD_PER] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_plld_per_data);
-       clks[BCM2835_PLLH_RCAL] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllh_rcal_data);
-       clks[BCM2835_PLLH_AUX] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllh_aux_data);
-       clks[BCM2835_PLLH_PIX] =3D
-               bcm2835_register_pll_divider(cprman, =
&bcm2835_pllh_pix_data);
-
-       clks[BCM2835_CLOCK_TIMER] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_timer_data);
-       clks[BCM2835_CLOCK_OTP] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_otp_data);
-       clks[BCM2835_CLOCK_TSENS] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_tsens_data);
-       clks[BCM2835_CLOCK_VPU] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_vpu_data);
-       clks[BCM2835_CLOCK_V3D] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data);
-       clks[BCM2835_CLOCK_ISP] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_isp_data);
-       clks[BCM2835_CLOCK_H264] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_h264_data);
-       clks[BCM2835_CLOCK_V3D] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data);
-       clks[BCM2835_CLOCK_SDRAM] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_sdram_data);
-       clks[BCM2835_CLOCK_UART] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_uart_data);
-       clks[BCM2835_CLOCK_VEC] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_vec_data);
-       clks[BCM2835_CLOCK_HSM] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_hsm_data);
-       clks[BCM2835_CLOCK_EMMC] =3D
-               bcm2835_register_clock(cprman, =
&bcm2835_clock_emmc_data);
-       clks[BCM2835_CLOCK_PCM] =3D
-               bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data);
+       /*  register pll */
+       for(i =3D 0; i< ARRAY_SIZE(bcm2835_plls); i++) {
+               if (!bcm2835_plls[i])
+                       continue;
+               clks[i] =3D bcm2835_register_pll( cprman, =
bcm2835_plls[i]);
+       }
+
+       /* register pll divider */
+       for(i =3D 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] =3D bcm2835_register_pll_divider(cprman,
+                                                      =
bcm2835_pll_divs[i]);
+       }
+
+       /* register clocks */
+       for(i =3D 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] =3D 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] =3D
-               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=20
missing from dt-bindings.h as well as the patch that adds all the
clock registers, which I had sent earlier.=

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 10.01.2016, at 19:56, Geert Uytterhoeven <geert@linux-m68k.org> =
wrote:
>=20
> Hi Martin,
>=20
> 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.
>>>=20
>>> If you fill in clks[] from a static table, you can take ARRAY_SIZE =
of
>>> the static table.
>>=20
>> You mean something like the below?
>> (note: copy/paste from console issues - spaces instead of tabs)
>=20
> More or less.
>=20
>> };
>>=20
>> 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 =3D {
>>        .max_fb_rate =3D BCM2835_MAX_FB_RATE,
>> };
>>=20
>> +static const struct bcm2835_pll_data *bcm2835_plls[] =3D {
>> +       [BCM2835_PLLA] =3D &bcm2835_plla_data,
>> +       [BCM2835_PLLB] =3D &bcm2835_pllb_data,
>> +       [BCM2835_PLLC] =3D &bcm2835_pllc_data,
>> +       [BCM2835_PLLD] =3D &bcm2835_plld_data,
>> +       [BCM2835_PLLH] =3D &bcm2835_pllh_data,
>> +};
>=20
> This is a sparse array?
Yes - for all.

>=20
>> 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 =3D max(ARRAY_SIZE(bcm2835_plls),
>> +                                max(ARRAY_SIZE(bcm2835_pll_divs),
>> +                                    ARRAY_SIZE(bcm2835_clks))) + 1;
>> +       size_t alloc =3D sizeof(*cprman) + clks_cnt * sizeof(*clks);
>> +       size_t i;
>=20
> 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 =
=E2=80=9Cfrequency shifts=E2=80=9D
for audio, which we want to avoid - this means we need another patch to
enable this MASH feature.

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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.

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

Hi Martin,

On Sun, Jan 10, 2016 at 8:07 PM, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> On 10.01.2016, at 19:56, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>>>> On 10.01.2016, at 14:02, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

On Sunday 10 January 2016 13:17:17 Martin Sperl wrote:
>=20
> 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 addre=
ss
> it.
>=20
> Maybe someone has a better idea for a pattern to use to achieve=20
> the required while maintaining =E2=80=9Csynchronization=E2=80=9D betw=
een 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 DI=
V
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.

=09Arnd

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Arnd Bergmann]

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

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 11.01.2016, at 14:38, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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...


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 11.01.2016, at 14:38, Arnd Bergmann <arnd@arndb.de> wrote:
>=20
> On Sunday 10 January 2016 13:17:17 Martin Sperl wrote:
>>=20
>> 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.
>>=20
>> Maybe someone has a better idea for a pattern to use to achieve=20
>> the required while maintaining =E2=80=9Csynchronization=E2=80=9D =
between defines
>> inside the dt-binding and the driver.
>=20
> It's funny to look at the register list:
>=20
> #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
>=20
> 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.
>=20
> I can see two possible ways forward:
>=20
> 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...

[PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> 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...