[PATCH 0/6] clk: bcm2835: fixes clk-bcm2835 driver issues

[PATCH 0/6] clk: bcm2835: fixes clk-bcm2835 driver issues

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

This patchset - as recommended by Stefan Wahren - is a split
of the original V5 patchset that now only includes bugfixes
to the clock driver not adding new clocks or functionality.

There will be another patchset that implements the
changes required to add all missing clocks of the SOC.

Martin Sperl (6):
  clk: bcm2835: pll_off should only set CM_PLL_ANARST
  clk: bcm2835: add locking to pll*_on/off methods
  clk: bcm2835: enable clocks that have been enabled by firmware
  clk: bcm2835: divider value has to be 1 or more
  clk: bcm2835: correctly enable fractional clock support
  clk: bcm2835: clean up coding style issues

 drivers/clk/bcm/clk-bcm2835.c |   87 +++++++++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 17 deletions(-)

--
1.7.10.4

[PATCH 1/6] clk: bcm2835: pll_off should only set CM_PLL_ANARST

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

bcm2835_pll_off is currently assigning CM_PLL_ANARST
to the control register.

This patch only sets the CM_PLL_ANARST bit
not resetting any of the other bits, which allows
restoring the register to its original value
via bcm2834_pll_on.

It also now locks during the read/modify/write cycle of
both registers.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
audio domain clocks")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 5747a9d..2b7c6af 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -913,8 +913,14 @@ static void bcm2835_pll_off(struct clk_hw *hw)
 	struct bcm2835_cprman *cprman = pll->cprman;
 	const struct bcm2835_pll_data *data = pll->data;

-	cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST);
-	cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN);
+	spin_lock(&cprman->regs_lock);
+	cprman_write(cprman, data->cm_ctrl_reg,
+		     cprman_read(cprman, data->cm_ctrl_reg) |
+		     CM_PLL_ANARST);
+	cprman_write(cprman, data->a2w_ctrl_reg,
+		     cprman_read(cprman, data->a2w_ctrl_reg) |
+		     A2W_PLL_CTRL_PWRDN);
+	spin_unlock(&cprman->regs_lock);
 }

 static int bcm2835_pll_on(struct clk_hw *hw)
--
1.7.10.4

Re: [PATCH 1/6] clk: bcm2835: pll_off should only set CM_PLL_ANARST

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> bcm2835_pll_off is currently assigning CM_PLL_ANARST
> to the control register.
>
> This patch only sets the CM_PLL_ANARST bit
> not resetting any of the other bits, which allows
> restoring the register to its original value
> via bcm2834_pll_on.
>
> It also now locks during the read/modify/write cycle of
> both registers.
>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
> audio domain clocks")
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/clk/bcm/clk-bcm2835.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 5747a9d..2b7c6af 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -913,8 +913,14 @@ static void bcm2835_pll_off(struct clk_hw *hw)
>  	struct bcm2835_cprman *cprman = pll->cprman;
>  	const struct bcm2835_pll_data *data = pll->data;
>
> -	cprman_write(cprman, data->cm_ctrl_reg, CM_PLL_ANARST);
> -	cprman_write(cprman, data->a2w_ctrl_reg, A2W_PLL_CTRL_PWRDN);
> +	spin_lock(&cprman->regs_lock);
> +	cprman_write(cprman, data->cm_ctrl_reg,
> +		     cprman_read(cprman, data->cm_ctrl_reg) |
> +		     CM_PLL_ANARST);
> +	cprman_write(cprman, data->a2w_ctrl_reg,
> +		     cprman_read(cprman, data->a2w_ctrl_reg) |
> +		     A2W_PLL_CTRL_PWRDN);
> +	spin_unlock(&cprman->regs_lock);
>  }

I think I see now: we're making sure that the hold_mask of a divider
doesn't get lost if we turn its source PLL off.

Reviewed-by: Eric Anholt <eric@anholt.net>

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

[PATCH 2/6] clk: bcm2835: add locking to pll*_on/off methods

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add missing locking to:
* bcm2835_pll_divider_on
* bcm2835_pll_divider_off
to protect the read modify write cycle for the
register access protecting both cm_ctl and a2w_ctl
registers.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
audio domain clocks")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 2b7c6af..30d6486 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1088,10 +1088,12 @@ static void bcm2835_pll_divider_off(struct clk_hw *hw)
 	struct bcm2835_cprman *cprman = divider->cprman;
 	const struct bcm2835_pll_divider_data *data = divider->data;

+	spin_lock(&cprman->regs_lock);
 	cprman_write(cprman, data->cm_reg,
 		     (cprman_read(cprman, data->cm_reg) &
 		      ~data->load_mask) | data->hold_mask);
 	cprman_write(cprman, data->a2w_reg, A2W_PLL_CHANNEL_DISABLE);
+	spin_unlock(&cprman->regs_lock);
 }

 static int bcm2835_pll_divider_on(struct clk_hw *hw)
@@ -1100,12 +1102,14 @@ static int bcm2835_pll_divider_on(struct clk_hw *hw)
 	struct bcm2835_cprman *cprman = divider->cprman;
 	const struct bcm2835_pll_divider_data *data = divider->data;

+	spin_lock(&cprman->regs_lock);
 	cprman_write(cprman, data->a2w_reg,
 		     cprman_read(cprman, data->a2w_reg) &
 		     ~A2W_PLL_CHANNEL_DISABLE);

 	cprman_write(cprman, data->cm_reg,
 		     cprman_read(cprman, data->cm_reg) & ~data->hold_mask);
+	spin_unlock(&cprman->regs_lock);

 	return 0;
 }
--
1.7.10.4

Re: [PATCH 2/6] clk: bcm2835: add locking to pll*_on/off methods

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> Add missing locking to:
> * bcm2835_pll_divider_on
> * bcm2835_pll_divider_off
> to protect the read modify write cycle for the
> register access protecting both cm_ctl and a2w_ctl
> registers.

s/ctl/reg/ in the commit message

bcm2835_pll_divider_set_rate() also does an RMW on cm_reg, why was it
left out?

Since the a2w reg is only modified by _on and _off, and the core holds
the prepare lock across those, I think it was already safe and doesn't
need to be mentioned here.

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

[PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

If a clock that has been enabled by the firmware gets disabled
by a driver this may right now result in a crash of the system
as then also the corresponding PLL_dividers as well as PLLs
get disabled (if not used) - some of which are used by the
VideoCore GPU (which also runs the firmware)

This patch prepares/enables those clocks that have been
configured by the firmware.

Whenever the clock framework implements either
CLK_IS_CRITICAL or HAND_OFF this can get changed to use this
new mechanism.

For this to be completely successful (i.e not missing a clock
and subsequently a pll) it is recommended to add all the known
clocks of the soc so that this can get applied to all clocks.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 30d6486..1fbb55d 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -37,6 +37,7 @@
  * generator).
  */

+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
 #include <linux/clk/bcm2835.h>
@@ -1478,6 +1479,7 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	struct clk_init_data init;
 	const char *parents[1 << CM_SRC_BITS];
 	size_t i;
+	struct clk *clk;

 	/*
 	 * Replace our "xosc" references with the oscillator's
@@ -1511,7 +1513,18 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	clock->data = data;
 	clock->hw.init = &init;

-	return devm_clk_register(cprman->dev, &clock->hw);
+	clk = devm_clk_register(cprman->dev, &clock->hw);
+	if (IS_ERR_OR_NULL(clk))
+		return clk;
+
+	/* enable/prepare if the clock is enabled by the firmware */
+	if (cprman_read(cprman, data->ctl_reg) & CM_ENABLE) {
+		dev_info(cprman->dev,
+			 "found firmware enabled clock %pC\n", clk);
+		clk_prepare_enable(clk);
+	}
+
+	return clk;
 }

 static int bcm2835_clk_probe(struct platform_device *pdev)
--
1.7.10.4

Re: [PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> If a clock that has been enabled by the firmware gets disabled
> by a driver this may right now result in a crash of the system
> as then also the corresponding PLL_dividers as well as PLLs
> get disabled (if not used) - some of which are used by the
> VideoCore GPU (which also runs the firmware)
>
> This patch prepares/enables those clocks that have been
> configured by the firmware.
>
> Whenever the clock framework implements either
> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this
> new mechanism.
>
> For this to be completely successful (i.e not missing a clock
> and subsequently a pll) it is recommended to add all the known
> clocks of the soc so that this can get applied to all clocks.

I think this makes sense to have, for now at least.

Reviewed-by: Eric Anholt <eric@anholt.net>

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

Re: [PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[Eric Anholt]

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

Eric Anholt <eric@anholt.net> writes:

> kernel@martin.sperl.org writes:
>
>> From: Martin Sperl <kernel@martin.sperl.org>
>>
>> If a clock that has been enabled by the firmware gets disabled
>> by a driver this may right now result in a crash of the system
>> as then also the corresponding PLL_dividers as well as PLLs
>> get disabled (if not used) - some of which are used by the
>> VideoCore GPU (which also runs the firmware)
>>
>> This patch prepares/enables those clocks that have been
>> configured by the firmware.
>>
>> Whenever the clock framework implements either
>> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this
>> new mechanism.
>>
>> For this to be completely successful (i.e not missing a clock
>> and subsequently a pll) it is recommended to add all the known
>> clocks of the soc so that this can get applied to all clocks.
>
> I think this makes sense to have, for now at least.
>
> Reviewed-by: Eric Anholt <eric@anholt.net>

Scratch that, this breaks display.  Since the clkgen clocks are flagged
as needing to be gated in order to change dividers, it means you can't
set clock rates for anything that was turned on at boot.

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

Re: [PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[Martin Sperl]

> On 17.03.2016, at 18:39, Eric Anholt <eric@anholt.net> wrote:
>=20
> Eric Anholt <eric@anholt.net> writes:
>=20
>> kernel@martin.sperl.org writes:
>>=20
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>=20
>>> If a clock that has been enabled by the firmware gets disabled
>>> by a driver this may right now result in a crash of the system
>>> as then also the corresponding PLL_dividers as well as PLLs
>>> get disabled (if not used) - some of which are used by the
>>> VideoCore GPU (which also runs the firmware)
>>>=20
>>> This patch prepares/enables those clocks that have been
>>> configured by the firmware.
>>>=20
>>> Whenever the clock framework implements either
>>> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this
>>> new mechanism.
>>>=20
>>> For this to be completely successful (i.e not missing a clock
>>> and subsequently a pll) it is recommended to add all the known
>>> clocks of the soc so that this can get applied to all clocks.
>>=20
>> I think this makes sense to have, for now at least.
>>=20
>> Reviewed-by: Eric Anholt <eric@anholt.net>
>=20
> Scratch that, this breaks display.  Since the clkgen clocks are =
flagged
> as needing to be gated in order to change dividers, it means you can't
> set clock rates for anything that was turned on at boot.

See that separate thread that triggered this:
  serial: clk: bcm2835: Strange effects when using aux-uart in console
and this patch fixes this issue.

To summarize the situation figured in the thread:
* you load the module

* you start using the tty (say by using "stty -F /dev/ttyAMA0")
* this opens the device
* this prepare the relevant clock (usage =3D 1)
* this prepares the parent pll-divider (usage =3D 1)
* this prepares the parent pll (usage =3D 1)

* you stop using the tty (stty closes the device)
* this release the clock
*   usage count drops to 0, so disable the clock
* this releases the parent pll-divider
*   usage count drops to 0, so disable the pll-div
* this releases the parent pll (and disables it as usage =3D 0)
*   usage count drops to 0, so disable the pll-div

* system crashes (with a bit of delay)

The prepare should just increase the usage so it never gets to a count =
of 0.

Maybe we need to use those "CLK_IS_CRITICAL=E2=80=9D =E2=80=9CHANDS_OFF=E2=
=80=9D flags instead?
(when/if they become available)

How do you want to solve that - I have not got a DSI display,=20
but HDMI continues to work...

Martin

Re: [PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[Eric Anholt]

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

Martin Sperl <kernel@martin.sperl.org> writes:

>> On 17.03.2016, at 18:39, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Eric Anholt <eric@anholt.net> writes:
>> 
>>> kernel@martin.sperl.org writes:
>>> 
>>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>> 
>>>> If a clock that has been enabled by the firmware gets disabled
>>>> by a driver this may right now result in a crash of the system
>>>> as then also the corresponding PLL_dividers as well as PLLs
>>>> get disabled (if not used) - some of which are used by the
>>>> VideoCore GPU (which also runs the firmware)
>>>> 
>>>> This patch prepares/enables those clocks that have been
>>>> configured by the firmware.
>>>> 
>>>> Whenever the clock framework implements either
>>>> CLK_IS_CRITICAL or HAND_OFF this can get changed to use this
>>>> new mechanism.
>>>> 
>>>> For this to be completely successful (i.e not missing a clock
>>>> and subsequently a pll) it is recommended to add all the known
>>>> clocks of the soc so that this can get applied to all clocks.
>>> 
>>> I think this makes sense to have, for now at least.
>>> 
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>> 
>> Scratch that, this breaks display.  Since the clkgen clocks are flagged
>> as needing to be gated in order to change dividers, it means you can't
>> set clock rates for anything that was turned on at boot.
>
> See that separate thread that triggered this:
>   serial: clk: bcm2835: Strange effects when using aux-uart in console
> and this patch fixes this issue.
>
> To summarize the situation figured in the thread:
> * you load the module
>
> * you start using the tty (say by using "stty -F /dev/ttyAMA0")
> * this opens the device
> * this prepare the relevant clock (usage = 1)
> * this prepares the parent pll-divider (usage = 1)
> * this prepares the parent pll (usage = 1)
>
> * you stop using the tty (stty closes the device)
> * this release the clock
> *   usage count drops to 0, so disable the clock
> * this releases the parent pll-divider
> *   usage count drops to 0, so disable the pll-div
> * this releases the parent pll (and disables it as usage = 0)
> *   usage count drops to 0, so disable the pll-div
>
> * system crashes (with a bit of delay)
>
> The prepare should just increase the usage so it never gets to a count of 0.
>
> Maybe we need to use those "CLK_IS_CRITICAL” “HANDS_OFF” flags instead?
> (when/if they become available)
>
> How do you want to solve that - I have not got a DSI display, 
> but HDMI continues to work...

We should just prepare the necessary divider, not the leaf clocks that
we actually want to control at runtime.

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

Re: [PATCH 3/6] clk: bcm2835: enable clocks that have been enabled by firmware

[Martin Sperl]

> On 17.03.2016, at 19:23, Eric Anholt <eric@anholt.net> wrote:
>=20
> Martin Sperl <kernel@martin.sperl.org> writes:
>>=20
>> See that separate thread that triggered this:
>>  serial: clk: bcm2835: Strange effects when using aux-uart in console
>> and this patch fixes this issue.
>>=20
>> To summarize the situation figured in the thread:
>> * you load the module
>>=20
>> * you start using the tty (say by using "stty -F /dev/ttyAMA0")
>> * this opens the device
>> * this prepare the relevant clock (usage =3D 1)
>> * this prepares the parent pll-divider (usage =3D 1)
>> * this prepares the parent pll (usage =3D 1)
>>=20
>> * you stop using the tty (stty closes the device)
>> * this release the clock
>> *   usage count drops to 0, so disable the clock
>> * this releases the parent pll-divider
>> *   usage count drops to 0, so disable the pll-div
>> * this releases the parent pll (and disables it as usage =3D 0)
>> *   usage count drops to 0, so disable the pll-div
>>=20
>> * system crashes (with a bit of delay)
>>=20
>> The prepare should just increase the usage so it never gets to a =
count of 0.
>>=20
>> Maybe we need to use those "CLK_IS_CRITICAL=E2=80=9D =E2=80=9CHANDS_OFF=
=E2=80=9D flags instead?
>> (when/if they become available)
>>=20
>> How do you want to solve that - I have not got a DSI display,=20
>> but HDMI continues to work...
>=20
> We should just prepare the necessary divider, not the leaf clocks that
> we actually want to control at runtime.

So how are we continuing here?
The reason why I am asking is that switching downstream to use the clock
driver result in the lockups also when using i2s/pcm.

I have seen that CLK_IS_CRITICAL has made it into the clk-next tree.
Should we use that for plls (or pll_dividers) that are running?

Something like this:
diff --git a/drivers/clk/bcm/clk-bcm2835.c =
b/drivers/clk/bcm/clk-bcm2835.c
index 35f8de7..c17019f 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1200,6 +1200,15 @@ bcm2835_register_pll_divider(struct =
bcm2835_cprman *cprman,
        divider->cprman =3D cprman;
        divider->data =3D data;

+       /* if the pll-divider is running, then mark is as critical */
+       if ((cprman_read(cprman, data->a2w_reg) &
+            A2W_PLL_CHANNEL_DISABLE) =3D=3D 0) {
+               dev_info(cprman->dev,
+                        "found enabled pll_div %s - marking it as =
critical\n",
+                        data->name);
+               init.flags |=3D CLK_IS_CRITICAL;
+       }
+
        clk =3D devm_clk_register(cprman->dev, &divider->div.hw);
        if (IS_ERR(clk))
                return clk;

At least for downstream this does work and gives the following messages:
[    2.861321] bcm2835-clk 20101000.cprman: found enabled pll_div =
plla_core - marking it as critical
[    2.875917] bcm2835-clk 20101000.cprman: found enabled pll_div =
pllb_arm - marking it as critical
[    2.961250] bcm2835-clk 20101000.cprman: found enabled pll_div =
pllc_core0 - marking it as critical
[    2.977317] bcm2835-clk 20101000.cprman: found enabled pll_div =
pllc_per - marking it as critical
[    2.993226] bcm2835-clk 20101000.cprman: found enabled pll_div =
plld_core - marking it as critical
[    3.010189] bcm2835-clk 20101000.cprman: found enabled pll_div =
plld_per - marking it as critical
[    3.024993] bcm2835-clk 20101000.cprman: found enabled pll_div =
pllh_pix - marking it as critical

Martin

[PATCH 4/6] clk: bcm2835: divider value has to be 1 or more

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Current clamping of a normal divider allows a value < 1 to be valid.

A divider of < 1 would actually only be possible if we had a PLL...

So this patch clamps the divider to 1.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
audio domain clocks")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 1fbb55d..edb1f74 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1194,8 +1194,9 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
 		div += unused_frac_mask + 1;
 	div &= ~unused_frac_mask;

-	/* Clamp to the limits. */
-	div = max(div, unused_frac_mask + 1);
+	/* clamp to min divider of 1 */
+	div = max_t(u32, div, 1 << CM_DIV_FRAC_BITS);
+	/* clamp to the highest possible fractional divider */
 	div = min_t(u32, div, GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1,
 				      CM_DIV_FRAC_BITS - data->frac_bits));

--
1.7.10.4

Re: [PATCH 4/6] clk: bcm2835: divider value has to be 1 or more

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> Current clamping of a normal divider allows a value < 1 to be valid.
>
> A divider of < 1 would actually only be possible if we had a PLL...
>
> So this patch clamps the divider to 1.
>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
> audio domain clocks")
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Reviewed-by: Eric Anholt <eric@anholt.net>

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

[PATCH 5/6] clk: bcm2835: correctly enable fractional clock support

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

The current driver calculates the clock divider with
fractional support enabled.

But it does not enable fractional support in the
control register itself resulting in an integer only divider,
but in clk_set_rate responds back the fractionally divided
clock frequency.

This patch enables fractional support in the control register
whenever there is a fractional bit set in the requested clock divider.

Mash clock limits are are also handled for the PWM clock
applying the correct divider limits (2 and max_int) applicable to
basic fractional divider support (mash order of 1).

It also adds locking to protect the read/modify/write cycle of
the register modification.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
audio domain clocks")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |   45 +++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index edb1f74..da77069 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -52,6 +52,7 @@
 #define CM_GNRICCTL		0x000
 #define CM_GNRICDIV		0x004
 # define CM_DIV_FRAC_BITS	12
+# define CM_DIV_FRAC_MASK	GENMASK(CM_DIV_FRAC_BITS - 1, 0)

 #define CM_VPUCTL		0x008
 #define CM_VPUDIV		0x00c
@@ -129,6 +130,7 @@
 # define CM_GATE			BIT(CM_GATE_BIT)
 # define CM_BUSY			BIT(7)
 # define CM_BUSYD			BIT(8)
+# define CM_FRAC			BIT(9)
 # define CM_SRC_SHIFT			0
 # define CM_SRC_BITS			4
 # define CM_SRC_MASK			0xf
@@ -648,6 +650,7 @@ struct bcm2835_clock_data {
 	u32 frac_bits;

 	bool is_vpu_clock;
+	bool is_mash_clock;
 };

 static const char *const bcm2835_clock_per_parents[] = {
@@ -829,6 +832,7 @@ static const struct bcm2835_clock_data bcm2835_clock_pwm_data = {
 	.div_reg = CM_PWMDIV,
 	.int_bits = 12,
 	.frac_bits = 12,
+	.is_mash_clock = true,
 };

 struct bcm2835_pll {
@@ -1184,7 +1188,7 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
 		GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0) >> 1;
 	u64 temp = (u64)parent_rate << CM_DIV_FRAC_BITS;
 	u64 rem;
-	u32 div;
+	u32 div, mindiv, maxdiv;

 	rem = do_div(temp, rate);
 	div = temp;
@@ -1194,11 +1198,23 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
 		div += unused_frac_mask + 1;
 	div &= ~unused_frac_mask;

-	/* clamp to min divider of 1 */
-	div = max_t(u32, div, 1 << CM_DIV_FRAC_BITS);
-	/* clamp to the highest possible fractional divider */
-	div = min_t(u32, div, GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1,
-				      CM_DIV_FRAC_BITS - data->frac_bits));
+	/* different clamping limits apply for a mash clock */
+	if (data->is_mash_clock) {
+		/* clamp to min divider of 2 */
+		mindiv = 2 << CM_DIV_FRAC_BITS;
+		/* clamp to the highest possible integer divider */
+		maxdiv = (BIT(data->int_bits) - 1) << CM_DIV_FRAC_BITS;
+	} else {
+		/* clamp to min divider of 1 */
+		mindiv = 1 << CM_DIV_FRAC_BITS;
+		/* clamp to the highest possible fractional divider */
+		maxdiv = GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1,
+				 CM_DIV_FRAC_BITS - data->frac_bits);
+	}
+
+	/* apply the clamping  limits */
+	div = max_t(u32, div, mindiv);
+	div = min_t(u32, div, maxdiv);

 	return div;
 }
@@ -1292,9 +1308,26 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 	struct bcm2835_cprman *cprman = clock->cprman;
 	const struct bcm2835_clock_data *data = clock->data;
 	u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate, false);
+	u32 ctl;
+
+	spin_lock(&cprman->regs_lock);
+
+	/*
+	 * Setting up frac support
+	 *
+	 * In principle it is recommended to stop/start the clock first,
+	 * but as we set CLK_SET_RATE_GATE during registration of the
+	 * clock this requirement should be take care of by the
+	 * clk-framework.
+	 */
+	ctl = cprman_read(cprman, data->ctl_reg) & ~CM_FRAC;
+	ctl |= (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0;
+	cprman_write(cprman, data->ctl_reg, ctl);

 	cprman_write(cprman, data->div_reg, div);

+	spin_unlock(&cprman->regs_lock);
+
 	return 0;
 }

--
1.7.10.4

Re: [PATCH 5/6] clk: bcm2835: correctly enable fractional clock support

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> The current driver calculates the clock divider with
> fractional support enabled.
>
> But it does not enable fractional support in the
> control register itself resulting in an integer only divider,
> but in clk_set_rate responds back the fractionally divided
> clock frequency.
>
> This patch enables fractional support in the control register
> whenever there is a fractional bit set in the requested clock divider.
>
> Mash clock limits are are also handled for the PWM clock
> applying the correct divider limits (2 and max_int) applicable to
> basic fractional divider support (mash order of 1).
>
> It also adds locking to protect the read/modify/write cycle of
> the register modification.
>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
> audio domain clocks")
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Rewrite of the commit message:

The driver has been using fractional dividers for all clocks with
fractional bits.  However, MASH clocks (those used to drive audio) only
do fractional division when in MASH mode, which is used to push clock
jitter out of the audio band.  The PWM clock (used to drive i2s audio)
is the only MASH clock currently enabled in the driver.

Additional MASH modes are available that widen the frequency range, but
only 1-level MASH is used for now, until we characterize some better
policy.

Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
audio domain clocks")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

> ---
>  drivers/clk/bcm/clk-bcm2835.c |   45 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index edb1f74..da77069 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -52,6 +52,7 @@
>  #define CM_GNRICCTL		0x000
>  #define CM_GNRICDIV		0x004
>  # define CM_DIV_FRAC_BITS	12
> +# define CM_DIV_FRAC_MASK	GENMASK(CM_DIV_FRAC_BITS - 1, 0)
>
>  #define CM_VPUCTL		0x008
>  #define CM_VPUDIV		0x00c
> @@ -129,6 +130,7 @@
>  # define CM_GATE			BIT(CM_GATE_BIT)
>  # define CM_BUSY			BIT(7)
>  # define CM_BUSYD			BIT(8)
> +# define CM_FRAC			BIT(9)

This is just the low bit of the 2-bit CM_MASH field at 9:10.  So you're
enabling MASH mode 1 in this patch.  Similarly, the subject of the patch
should be something like "Fix MASH-based fractional clock support for
PWM" since all the other clocks work with fractional already.

>  # define CM_SRC_SHIFT			0
>  # define CM_SRC_BITS			4
>  # define CM_SRC_MASK			0xf
> @@ -648,6 +650,7 @@ struct bcm2835_clock_data {
>  	u32 frac_bits;
>
>  	bool is_vpu_clock;
> +	bool is_mash_clock;
>  };
>
>  static const char *const bcm2835_clock_per_parents[] = {
> @@ -829,6 +832,7 @@ static const struct bcm2835_clock_data bcm2835_clock_pwm_data = {
>  	.div_reg = CM_PWMDIV,
>  	.int_bits = 12,
>  	.frac_bits = 12,
> +	.is_mash_clock = true,
>  };
>
>  struct bcm2835_pll {
> @@ -1184,7 +1188,7 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
>  		GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0) >> 1;
>  	u64 temp = (u64)parent_rate << CM_DIV_FRAC_BITS;
>  	u64 rem;
> -	u32 div;
> +	u32 div, mindiv, maxdiv;
>
>  	rem = do_div(temp, rate);
>  	div = temp;
> @@ -1194,11 +1198,23 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
>  		div += unused_frac_mask + 1;
>  	div &= ~unused_frac_mask;
>
> -	/* clamp to min divider of 1 */
> -	div = max_t(u32, div, 1 << CM_DIV_FRAC_BITS);
> -	/* clamp to the highest possible fractional divider */
> -	div = min_t(u32, div, GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1,
> -				      CM_DIV_FRAC_BITS - data->frac_bits));
> +	/* different clamping limits apply for a mash clock */
> +	if (data->is_mash_clock) {
> +		/* clamp to min divider of 2 */
> +		mindiv = 2 << CM_DIV_FRAC_BITS;
> +		/* clamp to the highest possible integer divider */
> +		maxdiv = (BIT(data->int_bits) - 1) << CM_DIV_FRAC_BITS;
> +	} else {
> +		/* clamp to min divider of 1 */
> +		mindiv = 1 << CM_DIV_FRAC_BITS;
> +		/* clamp to the highest possible fractional divider */
> +		maxdiv = GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1,
> +				 CM_DIV_FRAC_BITS - data->frac_bits);
> +	}
> +
> +	/* apply the clamping  limits */
> +	div = max_t(u32, div, mindiv);
> +	div = min_t(u32, div, maxdiv);
>
>  	return div;
>  }
> @@ -1292,9 +1308,26 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>  	struct bcm2835_cprman *cprman = clock->cprman;
>  	const struct bcm2835_clock_data *data = clock->data;
>  	u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate, false);
> +	u32 ctl;
> +
> +	spin_lock(&cprman->regs_lock);
> +
> +	/*
> +	 * Setting up frac support
> +	 *
> +	 * In principle it is recommended to stop/start the clock first,
> +	 * but as we set CLK_SET_RATE_GATE during registration of the
> +	 * clock this requirement should be take care of by the
> +	 * clk-framework.
> +	 */
> +	ctl = cprman_read(cprman, data->ctl_reg) & ~CM_FRAC;
> +	ctl |= (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0;
> +	cprman_write(cprman, data->ctl_reg, ctl);

This should all be under "if (data->is_mash_clock) {}" since it doesn't
do anything on non-mash clocks.

>
>  	cprman_write(cprman, data->div_reg, div);
>
> +	spin_unlock(&cprman->regs_lock);
> +
>  	return 0;
>  }

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

Re: [PATCH 5/6] clk: bcm2835: correctly enable fractional clock support

[Martin Sperl]

> On 29.02.2016, at 21:33, Eric Anholt <eric@anholt.net> wrote:
>=20
> kernel@martin.sperl.org writes:
>=20
>> From: Martin Sperl <kernel@martin.sperl.org>
>>=20
>> The current driver calculates the clock divider with
>> fractional support enabled.
>>=20
>> But it does not enable fractional support in the
>> control register itself resulting in an integer only divider,
>> but in clk_set_rate responds back the fractionally divided
>> clock frequency.
>>=20
>> This patch enables fractional support in the control register
>> whenever there is a fractional bit set in the requested clock =
divider.
>>=20
>> Mash clock limits are are also handled for the PWM clock
>> applying the correct divider limits (2 and max_int) applicable to
>> basic fractional divider support (mash order of 1).
>>=20
>> It also adds locking to protect the read/modify/write cycle of
>> the register modification.
>>=20
>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
>> audio domain clocks")
>>=20
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>=20
> Rewrite of the commit message:
>=20
> The driver has been using fractional dividers for all clocks with
> fractional bits.  However, MASH clocks (those used to drive audio) =
only
> do fractional division when in MASH mode, which is used to push clock
> jitter out of the audio band.  The PWM clock (used to drive i2s audio)
> is the only MASH clock currently enabled in the driver.
>=20
> Additional MASH modes are available that widen the frequency range, =
but
> only 1-level MASH is used for now, until we characterize some better
> policy.
>=20
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
> audio domain clocks=E2=80=9D)

That is not 100% true - non mash modes have CM_FRAC and CM_FRAC =3D =
CM_MASH0
so these map each other

And for a non-mash clock if CM_FRAC is not set then the factual divider
support is not used and only the integer portion is used.
This results in a frequency that is to high (as the frac bits are =
missing).

Just to giv an example of firmware set clock for the timer:
root@raspcm:~# head /sys/kernel/debug/clk/timer/current_*
=3D=3D> /sys/kernel/debug/clk/timer/current_divf <=3D=3D
819

=3D=3D> /sys/kernel/debug/clk/timer/current_divi <=3D=3D
19

=3D=3D> /sys/kernel/debug/clk/timer/current_frac <=3D=3D
1

=3D=3D> /sys/kernel/debug/clk/timer/current_parent <=3D=3D
xosc

root@raspcm:~# head /sys/kernel/debug/clk/timer/regdump
ctl =3D 0x00000291
div =3D 0x00013333

So this - non mash - clock has frac set by default
and this clock is set up by the firmware!

Similar things for: hsm and uart - all of them are =E2=80=9Cnormal=E2=80=9D=

clocks and they have the CM_FRAC bit set and divf > 0.

So the code essentially sets:
* CM_FRAC for =E2=80=9Cnormal=E2=80=9D clocks
* CM_MASH0 for mash clocks
when the divider has a fract component.

So I believe my description is correct.

But anyway: except for the pcm clocks clk_set_rate
is not really being used right now, as we still consume
the clocks set up by the firmware.

The comment portion about mash-order=3D1 could probably get added.

As for enabling higher order mash modes - at least in the audio
domain using I2S I have done some interresting measurements
making use of all the variations (and clock selections).

The result there was that - at least with the DAC that I=20
am using - the noise level varies widely between
different mash modes and clock dividers.

So the =E2=80=9Clowest=E2=80=9D noise showed the following combinations:
osc-mash3
pllc-mash2
osc-mash1
osc-non-frac (optimized so that we may use integer divider)
osc-mash2
pllc-mash1
pllc-mash3

So the "selection=E2=80=9D of clocks/divider/mash is really requiring
fine tuning and measurement - because there is no real pattern
visible.

Here a graph showing the audio spectrum for the different
modes with a 400Hz signal:
=
https://cloud.githubusercontent.com/assets/2638784/13278003/20e09002-dacd-=
11e5-8d62-17eb7118a4fa.jpg

>=20
> This is just the low bit of the 2-bit CM_MASH field at 9:10.  So =
you're
> enabling MASH mode 1 in this patch.  Similarly, the subject of the =
patch
> should be something like "Fix MASH-based fractional clock support for
> PWM" since all the other clocks work with fractional already.
>=20

As explained above: no they do not work without setting the FRAC bit!

Take again the example of timer:
when not using FRAC you get a timer clock of:
1010526Hz while with FRAC you get 1000002 Hz

So you NEED to set FRAC for all.
>>=20
>>=20
>> +	ctl =3D cprman_read(cprman, data->ctl_reg) & ~CM_FRAC;
>> +	ctl |=3D (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0;
>> +	cprman_write(cprman, data->ctl_reg, ctl);
>=20
> This should all be under "if (data->is_mash_clock) {}" since it =
doesn't
> do anything on non-mash clocks.
Again: without CM_FRAC you get a frequency that is too high, so it
needs to be implemented for all - independent of if it is a mash clock
or a FRAC clock.
The only cases where it does not apply is for clocks that have no fract
bits at all.

Re: [PATCH 5/6] clk: bcm2835: correctly enable fractional clock support

[Eric Anholt]

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

Martin Sperl <kernel@martin.sperl.org> writes:

>> On 29.02.2016, at 21:33, Eric Anholt <eric@anholt.net> wrote:
>> 
>> kernel@martin.sperl.org writes:
>> 
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>> 
>>> The current driver calculates the clock divider with
>>> fractional support enabled.
>>> 
>>> But it does not enable fractional support in the
>>> control register itself resulting in an integer only divider,
>>> but in clk_set_rate responds back the fractionally divided
>>> clock frequency.
>>> 
>>> This patch enables fractional support in the control register
>>> whenever there is a fractional bit set in the requested clock divider.
>>> 
>>> Mash clock limits are are also handled for the PWM clock
>>> applying the correct divider limits (2 and max_int) applicable to
>>> basic fractional divider support (mash order of 1).
>>> 
>>> It also adds locking to protect the read/modify/write cycle of
>>> the register modification.
>>> 
>>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
>>> audio domain clocks")
>>> 
>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> 
>> Rewrite of the commit message:
>> 
>> The driver has been using fractional dividers for all clocks with
>> fractional bits.  However, MASH clocks (those used to drive audio) only
>> do fractional division when in MASH mode, which is used to push clock
>> jitter out of the audio band.  The PWM clock (used to drive i2s audio)
>> is the only MASH clock currently enabled in the driver.
>> 
>> Additional MASH modes are available that widen the frequency range, but
>> only 1-level MASH is used for now, until we characterize some better
>> policy.
>> 
>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the
>> audio domain clocks”)
>
> That is not 100% true - non mash modes have CM_FRAC and CM_FRAC = CM_MASH0
> so these map each other

Once again, trusting the docs turns out to be a bad idea.  You're right,
the non-MASH clocks *do* have a bit 9 to enable fractional mode.  Sigh.

So, this patch is:

Reviewed-by: Eric Anholt <eric@anholt.net>

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

Re: [PATCH 5/6] clk: bcm2835: correctly enable fractional clock support

[Martin Sperl]

> On 01.03.2016, at 00:44, Eric Anholt <eric@anholt.net> wrote:
>=20
> Once again, trusting the docs turns out to be a bad idea.  You're =
right,
> the non-MASH clocks *do* have a bit 9 to enable fractional mode.  =
Sigh.
>=20
> So, this patch is:
>=20
> Reviewed-by: Eric Anholt <eric@anholt.net>

The only really reliable =E2=80=9Cdocs=E2=80=9D on registers I have =
found outside
of probably signing a NDA are those header files for the VC4 provided
by broadcom.=20

That is why I have taken the effort to translate those into something
much more easily readable/searchable (i.e html pages on the web).

And that is where I have =E2=80=9Cguessed=E2=80=9D all the information =
on the clock
tree structure.

The main thing that is sometimes missing from those documents is
some extra context like: what is that pllt clock with a mux of
8 parent clocks (not 4 or 16 as typical) and what do those
corresponding 4 counter really do?

See:
=
https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_pll=
tctl

Martin=

[PATCH 6/6] clk: bcm2835: clean up coding style issues

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Fix all the checkpatch complaints for clk-bcm2835.c

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/clk/bcm/clk-bcm2835.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index da77069..105a42f 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -12,9 +12,6 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
 /**
@@ -300,7 +297,7 @@
 struct bcm2835_cprman {
 	struct device *dev;
 	void __iomem *regs;
-	spinlock_t regs_lock;
+	spinlock_t regs_lock; /* spinlock for all clocks */
 	const char *osc_name;
 
 	struct clk_onecell_data onecell;
@@ -328,12 +325,12 @@ void __init bcm2835_init_clocks(void)
 	int ret;
 
 	clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL, CLK_IS_ROOT,
-					126000000);
+				      126000000);
 	if (IS_ERR(clk))
 		pr_err("apb_pclk not registered\n");
 
 	clk = clk_register_fixed_rate(NULL, "uart0_pclk", NULL, CLK_IS_ROOT,
-					3000000);
+				      3000000);
 	if (IS_ERR(clk))
 		pr_err("uart0_pclk not registered\n");
 	ret = clk_register_clkdev(clk, NULL, "20201000.uart");
@@ -341,7 +338,7 @@ void __init bcm2835_init_clocks(void)
 		pr_err("uart0_pclk alias not registered\n");
 
 	clk = clk_register_fixed_rate(NULL, "uart1_pclk", NULL, CLK_IS_ROOT,
-					125000000);
+				      125000000);
 	if (IS_ERR(clk))
 		pr_err("uart1_pclk not registered\n");
 	ret = clk_register_clkdev(clk, NULL, "20215000.uart");
@@ -1332,7 +1329,7 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 }
 
 static int bcm2835_clock_determine_rate(struct clk_hw *hw,
-		struct clk_rate_request *req)
+					struct clk_rate_request *req)
 {
 	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
 	struct clk_hw *parent, *best_parent = NULL;
@@ -1390,7 +1387,6 @@ static u8 bcm2835_clock_get_parent(struct clk_hw *hw)
 	return (src & CM_SRC_MASK) >> CM_SRC_SHIFT;
 }
 
-
 static const struct clk_ops bcm2835_clock_clk_ops = {
 	.is_prepared = bcm2835_clock_is_on,
 	.prepare = bcm2835_clock_on,
-- 
1.7.10.4

Re: [PATCH 6/6] clk: bcm2835: clean up coding style issues

[Eric Anholt]

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

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> Fix all the checkpatch complaints for clk-bcm2835.c
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/clk/bcm/clk-bcm2835.c |   14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index da77069..105a42f 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -12,9 +12,6 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
>   *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
>  /**
> @@ -300,7 +297,7 @@
>  struct bcm2835_cprman {
>  	struct device *dev;
>  	void __iomem *regs;
> -	spinlock_t regs_lock;
> +	spinlock_t regs_lock; /* spinlock for all clocks */
>  	const char *osc_name;

I don't think this comment adds anything at all, but I understand you're
trying to shut checkpatch up and I don't think there's much to say.  I'd
drop it, but I won't block the patch for it.  Everything else in the
patch looks good.

Acked-by: Eric Anholt <eric@anholt.net>

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