diff mbox

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

Message ID 1452331558-2520-3-git-send-email-kernel@martin.sperl.org (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Sperl Jan. 9, 2016, 9:25 a.m. UTC
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(-)

Comments

Arnd Bergmann Jan. 9, 2016, 8:56 p.m. UTC | #1
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
Geert Uytterhoeven Jan. 10, 2016, 9:30 a.m. UTC | #2
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
Martin Sperl Jan. 10, 2016, 10:55 a.m. UTC | #3
> 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
Mark Brown Jan. 10, 2016, 11:58 a.m. UTC | #4
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).
Martin Sperl Jan. 10, 2016, 12:17 p.m. UTC | #5
> 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.
Geert Uytterhoeven Jan. 10, 2016, 1:02 p.m. UTC | #6
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
Arnd Bergmann Jan. 11, 2016, 1:38 p.m. UTC | #7
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
Martin Sperl Jan. 11, 2016, 1:53 p.m. UTC | #8
> 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...
diff mbox

Patch

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