[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

With the new patch 2 inserted between my previous pair, I think this
should cover Martin's bugs with clock disabling.

I tested patch 2 to be important on the downstream kernel: with the
DPI panel support added there, I was losing ethernet (my only I/O)
when the HDMI HSM hanging off of PLLD_PER got disabled due to
EPROBE_DEFER.

Eric Anholt (3):
  clk: bcm2835: Mark the VPU clock as critical
  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

 drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

-- 
2.8.0.rc3

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

With the new patch 2 inserted between my previous pair, I think this
should cover Martin's bugs with clock disabling.

I tested patch 2 to be important on the downstream kernel: with the
DPI panel support added there, I was losing ethernet (my only I/O)
when the HDMI HSM hanging off of PLLD_PER got disabled due to
EPROBE_DEFER.

Eric Anholt (3):
  clk: bcm2835: Mark the VPU clock as critical
  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

 drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

-- 
2.8.0.rc3

[PATCH 1/3 v2] clk: bcm2835: Mark the VPU clock as critical

[Eric Anholt]

The VPU clock is also the clock for our AXI bus, so we really can't
disable it.  This might have happened during boot if, for example,
uart1 (aux_uart clock) probed and was then disabled before the other
consumers of the VPU clock had probed.

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

v2: Rewrite to use a .flags in bcm2835_clock_data, since other clocks
    will need this too.

 drivers/clk/bcm/clk-bcm2835.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 7a7970865c2d..d9db03cb3fd8 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -443,6 +443,8 @@ struct bcm2835_clock_data {
 	/* Number of fractional bits in the divider */
 	u32 frac_bits;
 
+	u32 flags;
+
 	bool is_vpu_clock;
 	bool is_mash_clock;
 };
@@ -1230,7 +1232,7 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	init.parent_names = parents;
 	init.num_parents = data->num_mux_parents;
 	init.name = data->name;
-	init.flags = CLK_IGNORE_UNUSED;
+	init.flags = data->flags | CLK_IGNORE_UNUSED;
 
 	if (data->is_vpu_clock) {
 		init.ops = &bcm2835_vpu_clock_clk_ops;
@@ -1649,6 +1651,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
 		.div_reg = CM_VPUDIV,
 		.int_bits = 12,
 		.frac_bits = 8,
+		.flags = CLK_IS_CRITICAL,
 		.is_vpu_clock = true),
 
 	/* clocks with per parent mux */
-- 
2.8.0.rc3

[PATCH 1/3 v2] clk: bcm2835: Mark the VPU clock as critical

[Eric Anholt]

The VPU clock is also the clock for our AXI bus, so we really can't
disable it.  This might have happened during boot if, for example,
uart1 (aux_uart clock) probed and was then disabled before the other
consumers of the VPU clock had probed.

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

v2: Rewrite to use a .flags in bcm2835_clock_data, since other clocks
    will need this too.

 drivers/clk/bcm/clk-bcm2835.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 7a7970865c2d..d9db03cb3fd8 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -443,6 +443,8 @@ struct bcm2835_clock_data {
 	/* Number of fractional bits in the divider */
 	u32 frac_bits;
 
+	u32 flags;
+
 	bool is_vpu_clock;
 	bool is_mash_clock;
 };
@@ -1230,7 +1232,7 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	init.parent_names = parents;
 	init.num_parents = data->num_mux_parents;
 	init.name = data->name;
-	init.flags = CLK_IGNORE_UNUSED;
+	init.flags = data->flags | CLK_IGNORE_UNUSED;
 
 	if (data->is_vpu_clock) {
 		init.ops = &bcm2835_vpu_clock_clk_ops;
@@ -1649,6 +1651,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
 		.div_reg = CM_VPUDIV,
 		.int_bits = 12,
 		.frac_bits = 8,
+		.flags = CLK_IS_CRITICAL,
 		.is_vpu_clock = true),
 
 	/* clocks with per parent mux */
-- 
2.8.0.rc3

[PATCH 2/3] clk: bcm2835: Mark GPIO clocks enabled at boot as critical.

[Eric Anholt]

These divide off of PLLD_PER and are used for the ethernet and wifi
PHYs source PLLs.  Neither of them is currently represented by a phy
device that would grab the clock for us.

This keeps other drivers from killing the networking PHYs when they
disable their own clocks and trigger PLLD_PER's refcount going to 0.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/clk/bcm/clk-bcm2835.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index d9db03cb3fd8..1091012ecec6 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1708,13 +1708,15 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
 		.div_reg = CM_GP1DIV,
 		.int_bits = 12,
 		.frac_bits = 12,
+		.flags = CLK_IS_CRITICAL,
 		.is_mash_clock = true),
 	[BCM2835_CLOCK_GP2]	= REGISTER_PER_CLK(
 		.name = "gp2",
 		.ctl_reg = CM_GP2CTL,
 		.div_reg = CM_GP2DIV,
 		.int_bits = 12,
-		.frac_bits = 12),
+		.frac_bits = 12,
+		.flags = CLK_IS_CRITICAL),
 
 	/* HDMI state machine */
 	[BCM2835_CLOCK_HSM]	= REGISTER_PER_CLK(
-- 
2.8.0.rc3

[PATCH 2/3] clk: bcm2835: Mark GPIO clocks enabled at boot as critical.

[Eric Anholt]

These divide off of PLLD_PER and are used for the ethernet and wifi
PHYs source PLLs.  Neither of them is currently represented by a phy
device that would grab the clock for us.

This keeps other drivers from killing the networking PHYs when they
disable their own clocks and trigger PLLD_PER's refcount going to 0.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/clk/bcm/clk-bcm2835.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index d9db03cb3fd8..1091012ecec6 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1708,13 +1708,15 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
 		.div_reg = CM_GP1DIV,
 		.int_bits = 12,
 		.frac_bits = 12,
+		.flags = CLK_IS_CRITICAL,
 		.is_mash_clock = true),
 	[BCM2835_CLOCK_GP2]	= REGISTER_PER_CLK(
 		.name = "gp2",
 		.ctl_reg = CM_GP2CTL,
 		.div_reg = CM_GP2DIV,
 		.int_bits = 12,
-		.frac_bits = 12),
+		.frac_bits = 12,
+		.flags = CLK_IS_CRITICAL),
 
 	/* HDMI state machine */
 	[BCM2835_CLOCK_HSM]	= REGISTER_PER_CLK(
-- 
2.8.0.rc3

[PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Eric Anholt]

If the firmware had set up a clock to source from PLLC, go along with
it.  But if we're looking for a new parent, we don't want to switch it
to PLLC because the firmware will force PLLC (and thus the AXI bus
clock) to different frequencies during over-temp/under-voltage,
without notification to Linux.

On my system, this moves the Linux-enabled HDMI state machine and DSI1
escape clock over to plld_per from pllc_per.  EMMC still ends up on
pllc_per, because the firmware had set it up to use that.

Signed-off-by: Eric Anholt <eric@anholt.net>
Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
---

No changes, just a resend.

 drivers/clk/bcm/clk-bcm2835.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 1091012ecec6..1d8f29ea9f69 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static bool
+bcm2835_clk_is_pllc(struct clk_hw *hw)
+{
+	if (!hw)
+		return false;
+
+	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;
+}
+
 static int bcm2835_clock_determine_rate(struct clk_hw *hw,
 					struct clk_rate_request *req)
 {
 	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
 	struct clk_hw *parent, *best_parent = NULL;
+	bool current_parent_is_pllc;
 	unsigned long rate, best_rate = 0;
 	unsigned long prate, best_prate = 0;
 	size_t i;
 	u32 div;
 
+	current_parent_is_pllc = bcm2835_clk_is_pllc(clk_hw_get_parent(hw));
+
 	/*
 	 * Select parent clock that results in the closest but lower rate
 	 */
@@ -1025,6 +1037,17 @@ static int bcm2835_clock_determine_rate(struct clk_hw *hw,
 		parent = clk_hw_get_parent_by_index(hw, i);
 		if (!parent)
 			continue;
+
+		/*
+		 * Don't choose a PLLC-derived clock as our parent
+		 * unless it had been manually set that way.  PLLC's
+		 * frequency gets adjusted by the firmware due to
+		 * over-temp or under-voltage conditions, without
+		 * prior notification to our clock consumer.
+		 */
+		if (bcm2835_clk_is_pllc(parent) && !current_parent_is_pllc)
+			continue;
+
 		prate = clk_hw_get_rate(parent);
 		div = bcm2835_clock_choose_div(hw, req->rate, prate, true);
 		rate = bcm2835_clock_rate_from_divisor(clock, prate, div);
-- 
2.8.0.rc3

[PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Eric Anholt]

If the firmware had set up a clock to source from PLLC, go along with
it.  But if we're looking for a new parent, we don't want to switch it
to PLLC because the firmware will force PLLC (and thus the AXI bus
clock) to different frequencies during over-temp/under-voltage,
without notification to Linux.

On my system, this moves the Linux-enabled HDMI state machine and DSI1
escape clock over to plld_per from pllc_per.  EMMC still ends up on
pllc_per, because the firmware had set it up to use that.

Signed-off-by: Eric Anholt <eric@anholt.net>
Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
---

No changes, just a resend.

 drivers/clk/bcm/clk-bcm2835.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 1091012ecec6..1d8f29ea9f69 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static bool
+bcm2835_clk_is_pllc(struct clk_hw *hw)
+{
+	if (!hw)
+		return false;
+
+	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;
+}
+
 static int bcm2835_clock_determine_rate(struct clk_hw *hw,
 					struct clk_rate_request *req)
 {
 	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
 	struct clk_hw *parent, *best_parent = NULL;
+	bool current_parent_is_pllc;
 	unsigned long rate, best_rate = 0;
 	unsigned long prate, best_prate = 0;
 	size_t i;
 	u32 div;
 
+	current_parent_is_pllc = bcm2835_clk_is_pllc(clk_hw_get_parent(hw));
+
 	/*
 	 * Select parent clock that results in the closest but lower rate
 	 */
@@ -1025,6 +1037,17 @@ static int bcm2835_clock_determine_rate(struct clk_hw *hw,
 		parent = clk_hw_get_parent_by_index(hw, i);
 		if (!parent)
 			continue;
+
+		/*
+		 * Don't choose a PLLC-derived clock as our parent
+		 * unless it had been manually set that way.  PLLC's
+		 * frequency gets adjusted by the firmware due to
+		 * over-temp or under-voltage conditions, without
+		 * prior notification to our clock consumer.
+		 */
+		if (bcm2835_clk_is_pllc(parent) && !current_parent_is_pllc)
+			continue;
+
 		prate = clk_hw_get_rate(parent);
 		div = bcm2835_clock_choose_div(hw, req->rate, prate, true);
 		rate = bcm2835_clock_rate_from_divisor(clock, prate, div);
-- 
2.8.0.rc3

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016 03:01, Eric Anholt wrote:
> With the new patch 2 inserted between my previous pair, I think this
> should cover Martin's bugs with clock disabling.
>
> I tested patch 2 to be important on the downstream kernel: with the
> DPI panel support added there, I was losing ethernet (my only I/O)
> when the HDMI HSM hanging off of PLLD_PER got disabled due to
> EPROBE_DEFER.
>
> Eric Anholt (3):
>    clk: bcm2835: Mark the VPU clock as critical
>    clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>    clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>
>   drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
>
I gave it a try - with all 3 patches applied I get the following enabled 
clocks:
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2

At least on my compute module gp1/gp2 is enabled, but there is no rate
set - so why is it marked as critical for all devices?
So why apply patch2 for all possible devices?

Loading/unloading the amba_pl011 module does not crash the system,
but a simple stty -F /dev/ttyAMA0 does crash the system!

Here the sequence:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  141.708453] Serial: AMBA PL011 UART driver
[  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2
root@raspcm:~# rmmod  amba_pl011
root@raspcm:~# dmesg -c
[  150.511248] Trying to free nonexistent resource 
<0000000020201000-0000000020201fff>
root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  159.385002] Serial: AMBA PL011 UART driver
[  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2
root@raspcm:~# stty -F /dev/ttyAMA0
speed 9600 baud; line = 0;
-brkint -imaxbel
root@raspcm:~# Timeout, server raspcm not responding.

The reason behind this is that the firmware pre-configured uart clock
looks like this:
root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
ctl = 0x00000296
div = 0x000a6aab
so it is configured to use plld_per (which itself is running, even if 
not enabled
in the kernel)

But as plld_per is not among the enabled clocks then plld_per
gets disabled as soon as the tty device is closed (by stty) and
this also disables plld...

Similar effect when using PCM/i2s and use speaker-test:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
modprobe snd-soc-hifiberry-dac
root@raspcm:~# dmesg
[   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
mapping ok
root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
[1] 579
root@raspcm:~#
speaker-test 1.0.28

Playback device is default
Stream parameters are 44100Hz, S16_LE, 2 channels
Sine wave rate is 440.0000Hz
Rate set to 44100Hz (requested 44100Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
  0 - Front Left
  1 - Front Right

root@raspcm:~#
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2
root@raspcm:~# kill %1
root@raspcm:~# Time per period = 106.889502
Timeout, server raspcm not responding.

You see that plld gets now used and when I kill speaker-test
the machine crashes again.

So this patchset does not really solve any of the problems that
I have reported either.

That is why my patchset has taken the "HAND_OFF" approach
instead (which still just hides some of the issues), but at least
it does not crash the system on the use of plld and it allows
for custom parent and mash selection.

In reality it would require consumers of the corresponding
parent clocks in the kernel (arm, ...) and the knowledge which
clocks are really needed by the firmware - i.e plld.

Note that the sdram clock is using plld_core parent!
root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
ctl = 0x00004006
div = 0x00003000
root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
166533331

and also hsm (probably hardware security module):
root@raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
ctl = 0x000002d6
div = 0x000030e0
root@raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
163551916

So turning plld off stops sdram and hsm - at least that is my
interpretation.

This means we need to define a clock property in firmware or
we need a ram node making use of "mmio-sram" maybe?

Marking sdram as "critical" or "hand_off" could also solve that
for the moment (but it does not solve all the other hidden
clock dependencies of the firmware)
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
clk_desc_array[] = {
                 .ctl_reg = CM_SDCCTL,
                 .div_reg = CM_SDCDIV,
                 .int_bits = 6,
-               .frac_bits = 0),
+               .frac_bits = 0,
+               .flags = CLK_IS_CRITICAL),
         [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
                 .name = "v3d",
                 .ctl_reg = CM_V3DCTL,

with this patch the system does no longer crash for the above
cases!

Still I would say that this should actually move to the dt to
correctly describe the HW.

Martin

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016 03:01, Eric Anholt wrote:
> With the new patch 2 inserted between my previous pair, I think this
> should cover Martin's bugs with clock disabling.
>
> I tested patch 2 to be important on the downstream kernel: with the
> DPI panel support added there, I was losing ethernet (my only I/O)
> when the HDMI HSM hanging off of PLLD_PER got disabled due to
> EPROBE_DEFER.
>
> Eric Anholt (3):
>    clk: bcm2835: Mark the VPU clock as critical
>    clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>    clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>
>   drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
>
I gave it a try - with all 3 patches applied I get the following enabled 
clocks:
root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2

At least on my compute module gp1/gp2 is enabled, but there is no rate
set - so why is it marked as critical for all devices?
So why apply patch2 for all possible devices?

Loading/unloading the amba_pl011 module does not crash the system,
but a simple stty -F /dev/ttyAMA0 does crash the system!

Here the sequence:
root at raspcm:~# dmesg -C
root at raspcm:~# modprobe amba_pl011
root at raspcm:~# dmesg -c
[  141.708453] Serial: AMBA PL011 UART driver
[  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2
root at raspcm:~# rmmod  amba_pl011
root at raspcm:~# dmesg -c
[  150.511248] Trying to free nonexistent resource 
<0000000020201000-0000000020201fff>
root at raspcm:~# modprobe amba_pl011
root at raspcm:~# dmesg -c
[  159.385002] Serial: AMBA PL011 UART driver
[  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2
root at raspcm:~# stty -F /dev/ttyAMA0
speed 9600 baud; line = 0;
-brkint -imaxbel
root at raspcm:~# Timeout, server raspcm not responding.

The reason behind this is that the firmware pre-configured uart clock
looks like this:
root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
ctl = 0x00000296
div = 0x000a6aab
so it is configured to use plld_per (which itself is running, even if 
not enabled
in the kernel)

But as plld_per is not among the enabled clocks then plld_per
gets disabled as soon as the tty device is closed (by stty) and
this also disables plld...

Similar effect when using PCM/i2s and use speaker-test:
root at raspcm:~# dmesg -C
root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
modprobe snd-soc-hifiberry-dac
root at raspcm:~# dmesg
[   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
mapping ok
root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
[1] 579
root at raspcm:~#
speaker-test 1.0.28

Playback device is default
Stream parameters are 44100Hz, S16_LE, 2 channels
Sine wave rate is 440.0000Hz
Rate set to 44100Hz (requested 44100Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
  0 - Front Left
  1 - Front Right

root at raspcm:~#
root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2
root at raspcm:~# kill %1
root at raspcm:~# Time per period = 106.889502
Timeout, server raspcm not responding.

You see that plld gets now used and when I kill speaker-test
the machine crashes again.

So this patchset does not really solve any of the problems that
I have reported either.

That is why my patchset has taken the "HAND_OFF" approach
instead (which still just hides some of the issues), but at least
it does not crash the system on the use of plld and it allows
for custom parent and mash selection.

In reality it would require consumers of the corresponding
parent clocks in the kernel (arm, ...) and the knowledge which
clocks are really needed by the firmware - i.e plld.

Note that the sdram clock is using plld_core parent!
root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
ctl = 0x00004006
div = 0x00003000
root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
166533331

and also hsm (probably hardware security module):
root at raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
ctl = 0x000002d6
div = 0x000030e0
root at raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
163551916

So turning plld off stops sdram and hsm - at least that is my
interpretation.

This means we need to define a clock property in firmware or
we need a ram node making use of "mmio-sram" maybe?

Marking sdram as "critical" or "hand_off" could also solve that
for the moment (but it does not solve all the other hidden
clock dependencies of the firmware)
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
clk_desc_array[] = {
                 .ctl_reg = CM_SDCCTL,
                 .div_reg = CM_SDCDIV,
                 .int_bits = 6,
-               .frac_bits = 0),
+               .frac_bits = 0,
+               .flags = CLK_IS_CRITICAL),
         [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
                 .name = "v3d",
                 .ctl_reg = CM_V3DCTL,

with this patch the system does no longer crash for the above
cases!

Still I would say that this should actually move to the dt to
correctly describe the HW.

Martin

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

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

> On 10.05.2016 03:01, Eric Anholt wrote:
>> With the new patch 2 inserted between my previous pair, I think this
>> should cover Martin's bugs with clock disabling.
>>
>> I tested patch 2 to be important on the downstream kernel: with the
>> DPI panel support added there, I was losing ethernet (my only I/O)
>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>> EPROBE_DEFER.
>>
>> Eric Anholt (3):
>>    clk: bcm2835: Mark the VPU clock as critical
>>    clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>    clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>
>>   drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
> I gave it a try - with all 3 patches applied I get the following enabled 
> clocks:
> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
> /sys/kernel/debug/clk/emmc/clk_enable_count:1
> /sys/kernel/debug/clk/gp1/clk_enable_count:1
> /sys/kernel/debug/clk/gp2/clk_enable_count:1
> /sys/kernel/debug/clk/osc/clk_enable_count:1
> /sys/kernel/debug/clk/pllc/clk_enable_count:2
> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>
> At least on my compute module gp1/gp2 is enabled, but there is no rate
> set - so why is it marked as critical for all devices?
> So why apply patch2 for all possible devices?

According to the CLK_IS_CRITICAL patches, the author intended critical
clocks not to use the included function for marking clocks as critical
From the DT.  I'm not sure why, but writing patches using that when they
say not to seemed like a waste.

We could check if gp1/gp2 are already on before marking them critical.

> Loading/unloading the amba_pl011 module does not crash the system,
> but a simple stty -F /dev/ttyAMA0 does crash the system!
>
> Here the sequence:
> root@raspcm:~# dmesg -C
> root@raspcm:~# modprobe amba_pl011
> root@raspcm:~# dmesg -c
> [  141.708453] Serial: AMBA PL011 UART driver
> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
> base_baud = 0) is a PL011 rev2
> root@raspcm:~# rmmod  amba_pl011
> root@raspcm:~# dmesg -c
> [  150.511248] Trying to free nonexistent resource 
> <0000000020201000-0000000020201fff>
> root@raspcm:~# modprobe amba_pl011
> root@raspcm:~# dmesg -c
> [  159.385002] Serial: AMBA PL011 UART driver
> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
> base_baud = 0) is a PL011 rev2
> root@raspcm:~# stty -F /dev/ttyAMA0
> speed 9600 baud; line = 0;
> -brkint -imaxbel
> root@raspcm:~# Timeout, server raspcm not responding.
>
> The reason behind this is that the firmware pre-configured uart clock
> looks like this:
> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
> ctl = 0x00000296
> div = 0x000a6aab
> so it is configured to use plld_per (which itself is running, even if 
> not enabled
> in the kernel)
>
> But as plld_per is not among the enabled clocks then plld_per
> gets disabled as soon as the tty device is closed (by stty) and
> this also disables plld...
>
> Similar effect when using PCM/i2s and use speaker-test:
> root@raspcm:~# dmesg -C
> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
> modprobe snd-soc-hifiberry-dac
> root@raspcm:~# dmesg
> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
> mapping ok
> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
> [1] 579
> root@raspcm:~#
> speaker-test 1.0.28
>
> Playback device is default
> Stream parameters are 44100Hz, S16_LE, 2 channels
> Sine wave rate is 440.0000Hz
> Rate set to 44100Hz (requested 44100Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
>   0 - Front Left
>   1 - Front Right
>
> root@raspcm:~#
> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
> /sys/kernel/debug/clk/emmc/clk_enable_count:1
> /sys/kernel/debug/clk/gp1/clk_enable_count:1
> /sys/kernel/debug/clk/gp2/clk_enable_count:1
> /sys/kernel/debug/clk/osc/clk_enable_count:2
> /sys/kernel/debug/clk/pcm/clk_enable_count:1
> /sys/kernel/debug/clk/pllc/clk_enable_count:2
> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
> /sys/kernel/debug/clk/plld/clk_enable_count:1
> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
> /sys/kernel/debug/clk/vpu/clk_enable_count:2
> root@raspcm:~# kill %1
> root@raspcm:~# Time per period = 106.889502
> Timeout, server raspcm not responding.
>
> You see that plld gets now used and when I kill speaker-test
> the machine crashes again.

Just so I can be clear here: What are you using to talk to the Pi?
Builtin USB ethernet?

> So this patchset does not really solve any of the problems that
> I have reported either.
>
> That is why my patchset has taken the "HAND_OFF" approach
> instead (which still just hides some of the issues), but at least
> it does not crash the system on the use of plld and it allows
> for custom parent and mash selection.

HAND_OFF sure doesn't look like it's landing in the next kernel, so
writing patches using it doesn't make much sense to me.

> In reality it would require consumers of the corresponding
> parent clocks in the kernel (arm, ...) and the knowledge which
> clocks are really needed by the firmware - i.e plld.
>
> Note that the sdram clock is using plld_core parent!
> root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
> ctl = 0x00004006
> div = 0x00003000
> root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
> 166533331

However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.

> and also hsm (probably hardware security module):
> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
> ctl = 0x000002d6
> div = 0x000030e0
> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
> 163551916

That's the HDMI state machine (there's even a comment saying so),
controlled by the vc4 driver.

> So turning plld off stops sdram and hsm - at least that is my
> interpretation.
>
> This means we need to define a clock property in firmware or
> we need a ram node making use of "mmio-sram" maybe?
>
> Marking sdram as "critical" or "hand_off" could also solve that
> for the moment (but it does not solve all the other hidden
> clock dependencies of the firmware)

If there are other hidden dependencies, then we should figure them out.

> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
> clk_desc_array[] = {
>                  .ctl_reg = CM_SDCCTL,
>                  .div_reg = CM_SDCDIV,
>                  .int_bits = 6,
> -               .frac_bits = 0),
> +               .frac_bits = 0,
> +               .flags = CLK_IS_CRITICAL),
>          [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>                  .name = "v3d",
>                  .ctl_reg = CM_V3DCTL,

The Pi foundation folks believe that the cprman SDRAM clock isn't ever
used (there's a separate PLL in the SDRAM controller, and cprman is only
intended for unused low-power states), and at least in your sample of
the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
would be really useful for debugging to look at your clk_summary
instead.

> Still I would say that this should actually move to the dt to
> correctly describe the HW.

If you created a series that *just* added critical support, using
assigned-clocks, and got the DT folks to agree to it, that would be fine
with me.

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

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

> On 10.05.2016 03:01, Eric Anholt wrote:
>> With the new patch 2 inserted between my previous pair, I think this
>> should cover Martin's bugs with clock disabling.
>>
>> I tested patch 2 to be important on the downstream kernel: with the
>> DPI panel support added there, I was losing ethernet (my only I/O)
>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>> EPROBE_DEFER.
>>
>> Eric Anholt (3):
>>    clk: bcm2835: Mark the VPU clock as critical
>>    clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>    clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>
>>   drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
> I gave it a try - with all 3 patches applied I get the following enabled 
> clocks:
> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
> /sys/kernel/debug/clk/emmc/clk_enable_count:1
> /sys/kernel/debug/clk/gp1/clk_enable_count:1
> /sys/kernel/debug/clk/gp2/clk_enable_count:1
> /sys/kernel/debug/clk/osc/clk_enable_count:1
> /sys/kernel/debug/clk/pllc/clk_enable_count:2
> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>
> At least on my compute module gp1/gp2 is enabled, but there is no rate
> set - so why is it marked as critical for all devices?
> So why apply patch2 for all possible devices?

According to the CLK_IS_CRITICAL patches, the author intended critical
clocks not to use the included function for marking clocks as critical
>From the DT.  I'm not sure why, but writing patches using that when they
say not to seemed like a waste.

We could check if gp1/gp2 are already on before marking them critical.

> Loading/unloading the amba_pl011 module does not crash the system,
> but a simple stty -F /dev/ttyAMA0 does crash the system!
>
> Here the sequence:
> root at raspcm:~# dmesg -C
> root at raspcm:~# modprobe amba_pl011
> root at raspcm:~# dmesg -c
> [  141.708453] Serial: AMBA PL011 UART driver
> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
> base_baud = 0) is a PL011 rev2
> root at raspcm:~# rmmod  amba_pl011
> root at raspcm:~# dmesg -c
> [  150.511248] Trying to free nonexistent resource 
> <0000000020201000-0000000020201fff>
> root at raspcm:~# modprobe amba_pl011
> root at raspcm:~# dmesg -c
> [  159.385002] Serial: AMBA PL011 UART driver
> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
> base_baud = 0) is a PL011 rev2
> root at raspcm:~# stty -F /dev/ttyAMA0
> speed 9600 baud; line = 0;
> -brkint -imaxbel
> root at raspcm:~# Timeout, server raspcm not responding.
>
> The reason behind this is that the firmware pre-configured uart clock
> looks like this:
> root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
> ctl = 0x00000296
> div = 0x000a6aab
> so it is configured to use plld_per (which itself is running, even if 
> not enabled
> in the kernel)
>
> But as plld_per is not among the enabled clocks then plld_per
> gets disabled as soon as the tty device is closed (by stty) and
> this also disables plld...
>
> Similar effect when using PCM/i2s and use speaker-test:
> root at raspcm:~# dmesg -C
> root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
> modprobe snd-soc-hifiberry-dac
> root at raspcm:~# dmesg
> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
> mapping ok
> root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
> [1] 579
> root at raspcm:~#
> speaker-test 1.0.28
>
> Playback device is default
> Stream parameters are 44100Hz, S16_LE, 2 channels
> Sine wave rate is 440.0000Hz
> Rate set to 44100Hz (requested 44100Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
>   0 - Front Left
>   1 - Front Right
>
> root at raspcm:~#
> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
> /sys/kernel/debug/clk/emmc/clk_enable_count:1
> /sys/kernel/debug/clk/gp1/clk_enable_count:1
> /sys/kernel/debug/clk/gp2/clk_enable_count:1
> /sys/kernel/debug/clk/osc/clk_enable_count:2
> /sys/kernel/debug/clk/pcm/clk_enable_count:1
> /sys/kernel/debug/clk/pllc/clk_enable_count:2
> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
> /sys/kernel/debug/clk/plld/clk_enable_count:1
> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
> /sys/kernel/debug/clk/vpu/clk_enable_count:2
> root at raspcm:~# kill %1
> root at raspcm:~# Time per period = 106.889502
> Timeout, server raspcm not responding.
>
> You see that plld gets now used and when I kill speaker-test
> the machine crashes again.

Just so I can be clear here: What are you using to talk to the Pi?
Builtin USB ethernet?

> So this patchset does not really solve any of the problems that
> I have reported either.
>
> That is why my patchset has taken the "HAND_OFF" approach
> instead (which still just hides some of the issues), but at least
> it does not crash the system on the use of plld and it allows
> for custom parent and mash selection.

HAND_OFF sure doesn't look like it's landing in the next kernel, so
writing patches using it doesn't make much sense to me.

> In reality it would require consumers of the corresponding
> parent clocks in the kernel (arm, ...) and the knowledge which
> clocks are really needed by the firmware - i.e plld.
>
> Note that the sdram clock is using plld_core parent!
> root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
> ctl = 0x00004006
> div = 0x00003000
> root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
> 166533331

However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.

> and also hsm (probably hardware security module):
> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
> ctl = 0x000002d6
> div = 0x000030e0
> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
> 163551916

That's the HDMI state machine (there's even a comment saying so),
controlled by the vc4 driver.

> So turning plld off stops sdram and hsm - at least that is my
> interpretation.
>
> This means we need to define a clock property in firmware or
> we need a ram node making use of "mmio-sram" maybe?
>
> Marking sdram as "critical" or "hand_off" could also solve that
> for the moment (but it does not solve all the other hidden
> clock dependencies of the firmware)

If there are other hidden dependencies, then we should figure them out.

> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
> clk_desc_array[] = {
>                  .ctl_reg = CM_SDCCTL,
>                  .div_reg = CM_SDCDIV,
>                  .int_bits = 6,
> -               .frac_bits = 0),
> +               .frac_bits = 0,
> +               .flags = CLK_IS_CRITICAL),
>          [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>                  .name = "v3d",
>                  .ctl_reg = CM_V3DCTL,

The Pi foundation folks believe that the cprman SDRAM clock isn't ever
used (there's a separate PLL in the SDRAM controller, and cprman is only
intended for unused low-power states), and at least in your sample of
the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
would be really useful for debugging to look at your clk_summary
instead.

> Still I would say that this should actually move to the dt to
> correctly describe the HW.

If you created a series that *just* added critical support, using
assigned-clocks, and got the DT folks to agree to it, that would be fine
with me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160510/e3c8773a/attachment.sig>

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
> 
> Martin Sperl <kernel@martin.sperl.org> writes:
> 
>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>> With the new patch 2 inserted between my previous pair, I think this
>>> should cover Martin's bugs with clock disabling.
>>> 
>>> I tested patch 2 to be important on the downstream kernel: with the
>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>> EPROBE_DEFER.
>>> 
>>> Eric Anholt (3):
>>>  clk: bcm2835: Mark the VPU clock as critical
>>>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>> 
>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>> I gave it a try - with all 3 patches applied I get the following enabled 
>> clocks:
>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> 
>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>> set - so why is it marked as critical for all devices?
>> So why apply patch2 for all possible devices?
> 
> According to the CLK_IS_CRITICAL patches, the author intended critical
> clocks not to use the included function for marking clocks as critical
> From the DT.  I'm not sure why, but writing patches using that when they
> say not to seemed like a waste.
> 
> We could check if gp1/gp2 are already on before marking them critical.
That may seem reasonable.
> 
>> Loading/unloading the amba_pl011 module does not crash the system,
>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>> 
>> Here the sequence:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  141.708453] Serial: AMBA PL011 UART driver
>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# rmmod  amba_pl011
>> root@raspcm:~# dmesg -c
>> [  150.511248] Trying to free nonexistent resource 
>> <0000000020201000-0000000020201fff>
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  159.385002] Serial: AMBA PL011 UART driver
>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# stty -F /dev/ttyAMA0
>> speed 9600 baud; line = 0;
>> -brkint -imaxbel
>> root@raspcm:~# Timeout, server raspcm not responding.
>> 
>> The reason behind this is that the firmware pre-configured uart clock
>> looks like this:
>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>> ctl = 0x00000296
>> div = 0x000a6aab
>> so it is configured to use plld_per (which itself is running, even if 
>> not enabled
>> in the kernel)
>> 
>> But as plld_per is not among the enabled clocks then plld_per
>> gets disabled as soon as the tty device is closed (by stty) and
>> this also disables plld...
>> 
>> Similar effect when using PCM/i2s and use speaker-test:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>> modprobe snd-soc-hifiberry-dac
>> root@raspcm:~# dmesg
>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>> mapping ok
>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>> [1] 579
>> root@raspcm:~#
>> speaker-test 1.0.28
>> 
>> Playback device is default
>> Stream parameters are 44100Hz, S16_LE, 2 channels
>> Sine wave rate is 440.0000Hz
>> Rate set to 44100Hz (requested 44100Hz)
>> Buffer size range from 128 to 131072
>> Period size range from 64 to 65536
>> Using max buffer size 131072
>> Periods = 4
>> was set period_size = 32768
>> was set buffer_size = 131072
>> 0 - Front Left
>> 1 - Front Right
>> 
>> root@raspcm:~#
>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> root@raspcm:~# kill %1
>> root@raspcm:~# Time per period = 106.889502
>> Timeout, server raspcm not responding.
>> 
>> You see that plld gets now used and when I kill speaker-test
>> the machine crashes again.
> 
> Just so I can be clear here: What are you using to talk to the Pi?
> Builtin USB ethernet?
Compute module with USB  Ethernet adapter, but I also got an 
enc28j60 connected via spi, but that is not enabled by default

> 
>> So this patchset does not really solve any of the problems that
>> I have reported either.
>> 
>> That is why my patchset has taken the "HAND_OFF" approach
>> instead (which still just hides some of the issues), but at least
>> it does not crash the system on the use of plld and it allows
>> for custom parent and mash selection.
> 
> HAND_OFF sure doesn't look like it's landing in the next kernel, so
> writing patches using it doesn't make much sense to me.

If it is critical or hands-off does not really make a difference...

>> In reality it would require consumers of the corresponding
>> parent clocks in the kernel (arm, ...) and the knowledge which
>> clocks are really needed by the firmware - i.e plld.
>> 
>> Note that the sdram clock is using plld_core parent!
>> root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>> ctl = 0x00004006
>> div = 0x00003000
>> root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>> 166533331
> 
> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
You are probably right there, but still there must be something
hidden that requires plld or plld_per or plld_core, that requires critical.

There must be some reason why it gets set up during the
boot process by the firmware.

> 
>> and also hsm (probably hardware security module):
>> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
>> ctl = 0x000002d6
>> div = 0x000030e0
>> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
>> 163551916
> 
> That's the HDMI state machine (there's even a comment saying so),
> controlled by the vc4 driver.
> 
>> So turning plld off stops sdram and hsm - at least that is my
>> interpretation.
>> 
>> This means we need to define a clock property in firmware or
>> we need a ram node making use of "mmio-sram" maybe?
>> 
>> Marking sdram as "critical" or "hand_off" could also solve that
>> for the moment (but it does not solve all the other hidden
>> clock dependencies of the firmware)
> 
> If there are other hidden dependencies, then we should figure them out.

But strangely the sdram (plus the below) is the only one with
plld that is enabled (unless it is one of the clocks we have not
added to the kernel side yet)

Maybe there is something that derives directly from plld_core
or any of the other plld-dividers?

Core would indicate anything central to the videocore...
Anyway both plld_core as well as plld_per ad well as both
Pll_dsi that are running by default (but I doubt that the
Dsi would be relevant)

I guess you are in a better situation to figure out which
hidden HW blocks uses plld...

> 
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
>> clk_desc_array[] = {
>>                .ctl_reg = CM_SDCCTL,
>>                .div_reg = CM_SDCDIV,
>>                .int_bits = 6,
>> -               .frac_bits = 0),
>> +               .frac_bits = 0,
>> +               .flags = CLK_IS_CRITICAL),
>>        [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>>                .name = "v3d",
>>                .ctl_reg = CM_V3DCTL,
> 
> The Pi foundation folks believe that the cprman SDRAM clock isn't ever
> used (there's a separate PLL in the SDRAM controller, and cprman is only
> intended for unused low-power states), and at least in your sample of
> the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
See my comment above - it must be configured for some reason
during the boot process by the firmware .


> would be really useful for debugging to look at your clk_summary
> instead.

Here you go:
   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 uart1_pclk                               0            0   125000000          0 0
 uart0_pclk                               0            0     3000000          0 0
 apb_pclk                                 0            0   126000000          0 0
 osc                                      2            2    19200000          0 0
    tsens                                 0            0     1920000          0 0
    vec                                   0            0    19200000          0 0
    otp                                   0            0     4800000          0 0
    timer                                 0            0     1000002          0 0
    pllh                                  0            0  1485000000          0 0
       pllh_pix_prediv                    0            0  1485000000          0 0
          pllh_pix                        0            0   148500000          0 0
       pllh_aux_prediv                    0            0     5800782          0 0
          pllh_aux                        0            0      580078          0 0
       pllh_rcal_prediv                   0            0     5800782          0 0
          pllh_rcal                       0            0      580078          0 0
    plld                                  2            2  1998399975          0 0
       plld_dsi1                          0            0     7806250          0 0
       plld_dsi0                          0            0     7806250          0 0
       plld_per                           1            1   499599994          0 0
          pwm                             1            1     9999958          0 0
          hsm                             0            0   163551916          0 0
          uart                            0            0     2997598          0 0
       plld_core                          1            1   499599994          0 0
          sdram                           1            1   166533331          0 0
    pllc                                  2            2  1998399975          0 0
       pllc_per                           1            1   999199988          0 0
          emmc                            1            1   249799997          0 0
       pllc_core2                         0            0     7806250          0 0
       pllc_core1                         0            0     7806250          0 0
       pllc_core0                         1            1   999199988          0 0
          vpu                             1            1   249799997          0 0
             aux_spi2                     0            0   249799997          0 0
             aux_spi1                     0            0   249799997          0 0
             aux_uart                     0            0   249799997          0 0
             peri_image                   0            0   249799997          0 0
    pllb                                  0            0  1399999951          0 0
       pllb_arm                           0            0   699999976          0 0
    plla                                  0            0  1998399975          0 0
       plla_ccp2                          0            0     7806250          0 0
       plla_dsi0                          0            0     7806250          0 0
       plla_per                           0            0     7806250          0 0
       plla_core                          0            0   999199988          0 0
          h264                            0            0   249799997          0 0
          isp                             0            0   249799997          0 0
          v3d                             0            0   249799997          0 0
 dsi1e                                    0            0           0          0 0
 dsi0e                                    0            0           0          0 0
 cam1                                     0            0           0          0 0
 cam0                                     0            0           0          0 0
 dpi                                      0            0           0          0 0
 tec                                      0            0           0          0 0
 smi                                      0            0           0          0 0
 slim                                     0            0           0          0 0
 gp2                                      1            1           0          0 0
 gp1                                      1            1           0          0 0
 gp0                                      0            0           0          0 0
 dft                                      0            0           0          0 0
 aveo                                     0            0           0          0 0
 pcm                                      0            0           0          0 0

(that is with the critical patch for sdram - so ignore the “enable_count”
for sdram)

> 
>> Still I would say that this should actually move to the dt to
>> correctly describe the HW.
> 
> If you created a series that *just* added critical support, using
> assigned-clocks, and got the DT folks to agree to it, that would be fine
> with me.

In the end I do not care for which patch gets in - I just want a solution
that does not crash and I guess that is also what the foundation
wants (and they want to move to the clock framework and I try to get
it there)

Martin

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
> 
> Martin Sperl <kernel@martin.sperl.org> writes:
> 
>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>> With the new patch 2 inserted between my previous pair, I think this
>>> should cover Martin's bugs with clock disabling.
>>> 
>>> I tested patch 2 to be important on the downstream kernel: with the
>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>> EPROBE_DEFER.
>>> 
>>> Eric Anholt (3):
>>>  clk: bcm2835: Mark the VPU clock as critical
>>>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>> 
>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>> I gave it a try - with all 3 patches applied I get the following enabled 
>> clocks:
>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> 
>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>> set - so why is it marked as critical for all devices?
>> So why apply patch2 for all possible devices?
> 
> According to the CLK_IS_CRITICAL patches, the author intended critical
> clocks not to use the included function for marking clocks as critical
> From the DT.  I'm not sure why, but writing patches using that when they
> say not to seemed like a waste.
> 
> We could check if gp1/gp2 are already on before marking them critical.
That may seem reasonable.
> 
>> Loading/unloading the amba_pl011 module does not crash the system,
>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>> 
>> Here the sequence:
>> root at raspcm:~# dmesg -C
>> root at raspcm:~# modprobe amba_pl011
>> root at raspcm:~# dmesg -c
>> [  141.708453] Serial: AMBA PL011 UART driver
>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root at raspcm:~# rmmod  amba_pl011
>> root at raspcm:~# dmesg -c
>> [  150.511248] Trying to free nonexistent resource 
>> <0000000020201000-0000000020201fff>
>> root at raspcm:~# modprobe amba_pl011
>> root at raspcm:~# dmesg -c
>> [  159.385002] Serial: AMBA PL011 UART driver
>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root at raspcm:~# stty -F /dev/ttyAMA0
>> speed 9600 baud; line = 0;
>> -brkint -imaxbel
>> root at raspcm:~# Timeout, server raspcm not responding.
>> 
>> The reason behind this is that the firmware pre-configured uart clock
>> looks like this:
>> root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>> ctl = 0x00000296
>> div = 0x000a6aab
>> so it is configured to use plld_per (which itself is running, even if 
>> not enabled
>> in the kernel)
>> 
>> But as plld_per is not among the enabled clocks then plld_per
>> gets disabled as soon as the tty device is closed (by stty) and
>> this also disables plld...
>> 
>> Similar effect when using PCM/i2s and use speaker-test:
>> root at raspcm:~# dmesg -C
>> root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>> modprobe snd-soc-hifiberry-dac
>> root at raspcm:~# dmesg
>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>> mapping ok
>> root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>> [1] 579
>> root at raspcm:~#
>> speaker-test 1.0.28
>> 
>> Playback device is default
>> Stream parameters are 44100Hz, S16_LE, 2 channels
>> Sine wave rate is 440.0000Hz
>> Rate set to 44100Hz (requested 44100Hz)
>> Buffer size range from 128 to 131072
>> Period size range from 64 to 65536
>> Using max buffer size 131072
>> Periods = 4
>> was set period_size = 32768
>> was set buffer_size = 131072
>> 0 - Front Left
>> 1 - Front Right
>> 
>> root at raspcm:~#
>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> root at raspcm:~# kill %1
>> root at raspcm:~# Time per period = 106.889502
>> Timeout, server raspcm not responding.
>> 
>> You see that plld gets now used and when I kill speaker-test
>> the machine crashes again.
> 
> Just so I can be clear here: What are you using to talk to the Pi?
> Builtin USB ethernet?
Compute module with USB  Ethernet adapter, but I also got an 
enc28j60 connected via spi, but that is not enabled by default

> 
>> So this patchset does not really solve any of the problems that
>> I have reported either.
>> 
>> That is why my patchset has taken the "HAND_OFF" approach
>> instead (which still just hides some of the issues), but at least
>> it does not crash the system on the use of plld and it allows
>> for custom parent and mash selection.
> 
> HAND_OFF sure doesn't look like it's landing in the next kernel, so
> writing patches using it doesn't make much sense to me.

If it is critical or hands-off does not really make a difference...

>> In reality it would require consumers of the corresponding
>> parent clocks in the kernel (arm, ...) and the knowledge which
>> clocks are really needed by the firmware - i.e plld.
>> 
>> Note that the sdram clock is using plld_core parent!
>> root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>> ctl = 0x00004006
>> div = 0x00003000
>> root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>> 166533331
> 
> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
You are probably right there, but still there must be something
hidden that requires plld or plld_per or plld_core, that requires critical.

There must be some reason why it gets set up during the
boot process by the firmware.

> 
>> and also hsm (probably hardware security module):
>> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
>> ctl = 0x000002d6
>> div = 0x000030e0
>> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
>> 163551916
> 
> That's the HDMI state machine (there's even a comment saying so),
> controlled by the vc4 driver.
> 
>> So turning plld off stops sdram and hsm - at least that is my
>> interpretation.
>> 
>> This means we need to define a clock property in firmware or
>> we need a ram node making use of "mmio-sram" maybe?
>> 
>> Marking sdram as "critical" or "hand_off" could also solve that
>> for the moment (but it does not solve all the other hidden
>> clock dependencies of the firmware)
> 
> If there are other hidden dependencies, then we should figure them out.

But strangely the sdram (plus the below) is the only one with
plld that is enabled (unless it is one of the clocks we have not
added to the kernel side yet)

Maybe there is something that derives directly from plld_core
or any of the other plld-dividers?

Core would indicate anything central to the videocore...
Anyway both plld_core as well as plld_per ad well as both
Pll_dsi that are running by default (but I doubt that the
Dsi would be relevant)

I guess you are in a better situation to figure out which
hidden HW blocks uses plld...

> 
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
>> clk_desc_array[] = {
>>                .ctl_reg = CM_SDCCTL,
>>                .div_reg = CM_SDCDIV,
>>                .int_bits = 6,
>> -               .frac_bits = 0),
>> +               .frac_bits = 0,
>> +               .flags = CLK_IS_CRITICAL),
>>        [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>>                .name = "v3d",
>>                .ctl_reg = CM_V3DCTL,
> 
> The Pi foundation folks believe that the cprman SDRAM clock isn't ever
> used (there's a separate PLL in the SDRAM controller, and cprman is only
> intended for unused low-power states), and at least in your sample of
> the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
See my comment above - it must be configured for some reason
during the boot process by the firmware .


> would be really useful for debugging to look at your clk_summary
> instead.

Here you go:
   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 uart1_pclk                               0            0   125000000          0 0
 uart0_pclk                               0            0     3000000          0 0
 apb_pclk                                 0            0   126000000          0 0
 osc                                      2            2    19200000          0 0
    tsens                                 0            0     1920000          0 0
    vec                                   0            0    19200000          0 0
    otp                                   0            0     4800000          0 0
    timer                                 0            0     1000002          0 0
    pllh                                  0            0  1485000000          0 0
       pllh_pix_prediv                    0            0  1485000000          0 0
          pllh_pix                        0            0   148500000          0 0
       pllh_aux_prediv                    0            0     5800782          0 0
          pllh_aux                        0            0      580078          0 0
       pllh_rcal_prediv                   0            0     5800782          0 0
          pllh_rcal                       0            0      580078          0 0
    plld                                  2            2  1998399975          0 0
       plld_dsi1                          0            0     7806250          0 0
       plld_dsi0                          0            0     7806250          0 0
       plld_per                           1            1   499599994          0 0
          pwm                             1            1     9999958          0 0
          hsm                             0            0   163551916          0 0
          uart                            0            0     2997598          0 0
       plld_core                          1            1   499599994          0 0
          sdram                           1            1   166533331          0 0
    pllc                                  2            2  1998399975          0 0
       pllc_per                           1            1   999199988          0 0
          emmc                            1            1   249799997          0 0
       pllc_core2                         0            0     7806250          0 0
       pllc_core1                         0            0     7806250          0 0
       pllc_core0                         1            1   999199988          0 0
          vpu                             1            1   249799997          0 0
             aux_spi2                     0            0   249799997          0 0
             aux_spi1                     0            0   249799997          0 0
             aux_uart                     0            0   249799997          0 0
             peri_image                   0            0   249799997          0 0
    pllb                                  0            0  1399999951          0 0
       pllb_arm                           0            0   699999976          0 0
    plla                                  0            0  1998399975          0 0
       plla_ccp2                          0            0     7806250          0 0
       plla_dsi0                          0            0     7806250          0 0
       plla_per                           0            0     7806250          0 0
       plla_core                          0            0   999199988          0 0
          h264                            0            0   249799997          0 0
          isp                             0            0   249799997          0 0
          v3d                             0            0   249799997          0 0
 dsi1e                                    0            0           0          0 0
 dsi0e                                    0            0           0          0 0
 cam1                                     0            0           0          0 0
 cam0                                     0            0           0          0 0
 dpi                                      0            0           0          0 0
 tec                                      0            0           0          0 0
 smi                                      0            0           0          0 0
 slim                                     0            0           0          0 0
 gp2                                      1            1           0          0 0
 gp1                                      1            1           0          0 0
 gp0                                      0            0           0          0 0
 dft                                      0            0           0          0 0
 aveo                                     0            0           0          0 0
 pcm                                      0            0           0          0 0

(that is with the critical patch for sdram - so ignore the ?enable_count?
for sdram)

> 
>> Still I would say that this should actually move to the dt to
>> correctly describe the HW.
> 
> If you created a series that *just* added critical support, using
> assigned-clocks, and got the DT folks to agree to it, that would be fine
> with me.

In the end I do not care for which patch gets in - I just want a solution
that does not crash and I guess that is also what the foundation
wants (and they want to move to the clock framework and I try to get
it there)

Martin

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> 
> 
>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> writes:
>> 
>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>> With the new patch 2 inserted between my previous pair, I think this
>>>> should cover Martin's bugs with clock disabling.
>>>> 
>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>> EPROBE_DEFER.
>>>> 
>>>> Eric Anholt (3):
>>>> clk: bcm2835: Mark the VPU clock as critical
>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>> 
>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>> clocks:
>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> 
>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>> set - so why is it marked as critical for all devices?
>>> So why apply patch2 for all possible devices?
>> 
>> According to the CLK_IS_CRITICAL patches, the author intended critical
>> clocks not to use the included function for marking clocks as critical
>> From the DT.  I'm not sure why, but writing patches using that when they
>> say not to seemed like a waste.
>> 
>> We could check if gp1/gp2 are already on before marking them critical.
> That may seem reasonable.
>> 
>>> Loading/unloading the amba_pl011 module does not crash the system,
>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>> 
>>> Here the sequence:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  141.708453] Serial: AMBA PL011 UART driver
>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# rmmod  amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  150.511248] Trying to free nonexistent resource 
>>> <0000000020201000-0000000020201fff>
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  159.385002] Serial: AMBA PL011 UART driver
>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>> speed 9600 baud; line = 0;
>>> -brkint -imaxbel
>>> root@raspcm:~# Timeout, server raspcm not responding.
>>> 
>>> The reason behind this is that the firmware pre-configured uart clock
>>> looks like this:
>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>> ctl = 0x00000296
>>> div = 0x000a6aab
>>> so it is configured to use plld_per (which itself is running, even if 
>>> not enabled
>>> in the kernel)
>>> 
>>> But as plld_per is not among the enabled clocks then plld_per
>>> gets disabled as soon as the tty device is closed (by stty) and
>>> this also disables plld...
>>> 
>>> Similar effect when using PCM/i2s and use speaker-test:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>> modprobe snd-soc-hifiberry-dac
>>> root@raspcm:~# dmesg
>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>> mapping ok
>>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>> [1] 579
>>> root@raspcm:~#
>>> speaker-test 1.0.28
>>> 
>>> Playback device is default
>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>> Sine wave rate is 440.0000Hz
>>> Rate set to 44100Hz (requested 44100Hz)
>>> Buffer size range from 128 to 131072
>>> Period size range from 64 to 65536
>>> Using max buffer size 131072
>>> Periods = 4
>>> was set period_size = 32768
>>> was set buffer_size = 131072
>>> 0 - Front Left
>>> 1 - Front Right
>>> 
>>> root@raspcm:~#
>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> root@raspcm:~# kill %1
>>> root@raspcm:~# Time per period = 106.889502
>>> Timeout, server raspcm not responding.
>>> 
>>> You see that plld gets now used and when I kill speaker-test
>>> the machine crashes again.
>> 
>> Just so I can be clear here: What are you using to talk to the Pi?
>> Builtin USB ethernet?
> Compute module with USB  Ethernet adapter, but I also got an 
> enc28j60 connected via spi, but that is not enabled by default
> 
>> 
>>> So this patchset does not really solve any of the problems that
>>> I have reported either.
>>> 
>>> That is why my patchset has taken the "HAND_OFF" approach
>>> instead (which still just hides some of the issues), but at least
>>> it does not crash the system on the use of plld and it allows
>>> for custom parent and mash selection.
>> 
>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>> writing patches using it doesn't make much sense to me.
> 
> If it is critical or hands-off does not really make a difference...
> 
>>> In reality it would require consumers of the corresponding
>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>> clocks are really needed by the firmware - i.e plld.
>>> 
>>> Note that the sdram clock is using plld_core parent!
>>> root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>> ctl = 0x00004006
>>> div = 0x00003000
>>> root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>> 166533331
>> 
>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
> You are probably right there, but still there must be something
> hidden that requires plld or plld_per or plld_core, that requires critical.
> 
> There must be some reason why it gets set up during the
> boot process by the firmware.
> 

Correction:

Actually the SDC control register contains some more bits compared
to most other clock control registers:
CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)

See also:
https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl

If I poke that register (0x201011a8) from userspace with 0x5a000000
then the machine crashes as well - so I assume these bits control
the custom SD-Ram PLL - unfortunately it still does not say which
PLL is used.

Eric, you may have access to more data regarding this register -
maybe we need to hand this register as a special clock?
and/or expose it as an additional pll?

Note that I also tried disabling PLLD_CORE and the system crashed as well
(root@raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
/dev/mem opened.
Memory mapped at address 0xb6fdd000.
Value at address 0x2010110C (0xb6fdd10c): 0x20A
root@raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
/dev/mem opened.
Memory mapped at address 0xb6fe7000.
Value at address 0x2010110C (0xb6fe710c): 0x20A
Written 0x5A00022A; readback 0x22A
root@raspcm:~# Write failed: Broken pipe

(requires /dev/mem without restrictions)

So this means that it is plld_core related - at least as of now
and the sdram pll currently derives from PLLD_core.

@Eric: so please look at the hld (which you have hinted you have
access to) and come up with something better for sdram

As far as I can tell at the very least the sdram clock is critical
and should be marked as such…

Martin

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> 
> 
>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> writes:
>> 
>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>> With the new patch 2 inserted between my previous pair, I think this
>>>> should cover Martin's bugs with clock disabling.
>>>> 
>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>> EPROBE_DEFER.
>>>> 
>>>> Eric Anholt (3):
>>>> clk: bcm2835: Mark the VPU clock as critical
>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>> 
>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>> clocks:
>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> 
>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>> set - so why is it marked as critical for all devices?
>>> So why apply patch2 for all possible devices?
>> 
>> According to the CLK_IS_CRITICAL patches, the author intended critical
>> clocks not to use the included function for marking clocks as critical
>> From the DT.  I'm not sure why, but writing patches using that when they
>> say not to seemed like a waste.
>> 
>> We could check if gp1/gp2 are already on before marking them critical.
> That may seem reasonable.
>> 
>>> Loading/unloading the amba_pl011 module does not crash the system,
>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>> 
>>> Here the sequence:
>>> root at raspcm:~# dmesg -C
>>> root at raspcm:~# modprobe amba_pl011
>>> root at raspcm:~# dmesg -c
>>> [  141.708453] Serial: AMBA PL011 UART driver
>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root at raspcm:~# rmmod  amba_pl011
>>> root at raspcm:~# dmesg -c
>>> [  150.511248] Trying to free nonexistent resource 
>>> <0000000020201000-0000000020201fff>
>>> root at raspcm:~# modprobe amba_pl011
>>> root at raspcm:~# dmesg -c
>>> [  159.385002] Serial: AMBA PL011 UART driver
>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root at raspcm:~# stty -F /dev/ttyAMA0
>>> speed 9600 baud; line = 0;
>>> -brkint -imaxbel
>>> root at raspcm:~# Timeout, server raspcm not responding.
>>> 
>>> The reason behind this is that the firmware pre-configured uart clock
>>> looks like this:
>>> root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>> ctl = 0x00000296
>>> div = 0x000a6aab
>>> so it is configured to use plld_per (which itself is running, even if 
>>> not enabled
>>> in the kernel)
>>> 
>>> But as plld_per is not among the enabled clocks then plld_per
>>> gets disabled as soon as the tty device is closed (by stty) and
>>> this also disables plld...
>>> 
>>> Similar effect when using PCM/i2s and use speaker-test:
>>> root at raspcm:~# dmesg -C
>>> root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>> modprobe snd-soc-hifiberry-dac
>>> root at raspcm:~# dmesg
>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>> mapping ok
>>> root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>> [1] 579
>>> root at raspcm:~#
>>> speaker-test 1.0.28
>>> 
>>> Playback device is default
>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>> Sine wave rate is 440.0000Hz
>>> Rate set to 44100Hz (requested 44100Hz)
>>> Buffer size range from 128 to 131072
>>> Period size range from 64 to 65536
>>> Using max buffer size 131072
>>> Periods = 4
>>> was set period_size = 32768
>>> was set buffer_size = 131072
>>> 0 - Front Left
>>> 1 - Front Right
>>> 
>>> root at raspcm:~#
>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> root at raspcm:~# kill %1
>>> root at raspcm:~# Time per period = 106.889502
>>> Timeout, server raspcm not responding.
>>> 
>>> You see that plld gets now used and when I kill speaker-test
>>> the machine crashes again.
>> 
>> Just so I can be clear here: What are you using to talk to the Pi?
>> Builtin USB ethernet?
> Compute module with USB  Ethernet adapter, but I also got an 
> enc28j60 connected via spi, but that is not enabled by default
> 
>> 
>>> So this patchset does not really solve any of the problems that
>>> I have reported either.
>>> 
>>> That is why my patchset has taken the "HAND_OFF" approach
>>> instead (which still just hides some of the issues), but at least
>>> it does not crash the system on the use of plld and it allows
>>> for custom parent and mash selection.
>> 
>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>> writing patches using it doesn't make much sense to me.
> 
> If it is critical or hands-off does not really make a difference...
> 
>>> In reality it would require consumers of the corresponding
>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>> clocks are really needed by the firmware - i.e plld.
>>> 
>>> Note that the sdram clock is using plld_core parent!
>>> root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>> ctl = 0x00004006
>>> div = 0x00003000
>>> root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>> 166533331
>> 
>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
> You are probably right there, but still there must be something
> hidden that requires plld or plld_per or plld_core, that requires critical.
> 
> There must be some reason why it gets set up during the
> boot process by the firmware.
> 

Correction:

Actually the SDC control register contains some more bits compared
to most other clock control registers:
CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)

See also:
https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl

If I poke that register (0x201011a8) from userspace with 0x5a000000
then the machine crashes as well - so I assume these bits control
the custom SD-Ram PLL - unfortunately it still does not say which
PLL is used.

Eric, you may have access to more data regarding this register -
maybe we need to hand this register as a special clock?
and/or expose it as an additional pll?

Note that I also tried disabling PLLD_CORE and the system crashed as well
(root at raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
/dev/mem opened.
Memory mapped at address 0xb6fdd000.
Value at address 0x2010110C (0xb6fdd10c): 0x20A
root at raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
/dev/mem opened.
Memory mapped at address 0xb6fe7000.
Value at address 0x2010110C (0xb6fe710c): 0x20A
Written 0x5A00022A; readback 0x22A
root at raspcm:~# Write failed: Broken pipe

(requires /dev/mem without restrictions)

So this means that it is plld_core related - at least as of now
and the sdram pll currently derives from PLLD_core.

@Eric: so please look at the hld (which you have hinted you have
access to) and come up with something better for sdram

As far as I can tell at the very least the sdram clock is critical
and should be marked as such?

Martin

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 11.05.2016, at 10:21, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# rmmod  amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <0000000020201000-0000000020201fff>
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root@raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x00000296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>>> modprobe snd-soc-hifiberry-dac
>>>> root@raspcm:~# dmesg
>>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>>> mapping ok
>>>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>>> [1] 579
>>>> root@raspcm:~#
>>>> speaker-test 1.0.28
>>>> 
>>>> Playback device is default
>>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>>> Sine wave rate is 440.0000Hz
>>>> Rate set to 44100Hz (requested 44100Hz)
>>>> Buffer size range from 128 to 131072
>>>> Period size range from 64 to 65536
>>>> Using max buffer size 131072
>>>> Periods = 4
>>>> was set period_size = 32768
>>>> was set buffer_size = 131072
>>>> 0 - Front Left
>>>> 1 - Front Right
>>>> 
>>>> root@raspcm:~#
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> root@raspcm:~# kill %1
>>>> root@raspcm:~# Time per period = 106.889502
>>>> Timeout, server raspcm not responding.
>>>> 
>>>> You see that plld gets now used and when I kill speaker-test
>>>> the machine crashes again.
>>> 
>>> Just so I can be clear here: What are you using to talk to the Pi?
>>> Builtin USB ethernet?
>> Compute module with USB  Ethernet adapter, but I also got an 
>> enc28j60 connected via spi, but that is not enabled by default
>> 
>>> 
>>>> So this patchset does not really solve any of the problems that
>>>> I have reported either.
>>>> 
>>>> That is why my patchset has taken the "HAND_OFF" approach
>>>> instead (which still just hides some of the issues), but at least
>>>> it does not crash the system on the use of plld and it allows
>>>> for custom parent and mash selection.
>>> 
>>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>>> writing patches using it doesn't make much sense to me.
>> 
>> If it is critical or hands-off does not really make a difference...
>> 
>>>> In reality it would require consumers of the corresponding
>>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>>> clocks are really needed by the firmware - i.e plld.
>>>> 
>>>> Note that the sdram clock is using plld_core parent!
>>>> root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>>> ctl = 0x00004006
>>>> div = 0x00003000
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>>> 166533331
>>> 
>>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
>> You are probably right there, but still there must be something
>> hidden that requires plld or plld_per or plld_core, that requires critical.
>> 
>> There must be some reason why it gets set up during the
>> boot process by the firmware.
>> 
> 
> Correction:
> 
> Actually the SDC control register contains some more bits compared
> to most other clock control registers:
> CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)
> 
> See also:
> https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl
> 
> If I poke that register (0x201011a8) from userspace with 0x5a000000
> then the machine crashes as well - so I assume these bits control
> the custom SD-Ram PLL - unfortunately it still does not say which
> PLL is used.
> 
> Eric, you may have access to more data regarding this register -
> maybe we need to hand this register as a special clock?
> and/or expose it as an additional pll?
> 
> Note that I also tried disabling PLLD_CORE and the system crashed as well
> (root@raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
> /dev/mem opened.
> Memory mapped at address 0xb6fdd000.
> Value at address 0x2010110C (0xb6fdd10c): 0x20A
> root@raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
> /dev/mem opened.
> Memory mapped at address 0xb6fe7000.
> Value at address 0x2010110C (0xb6fe710c): 0x20A
> Written 0x5A00022A; readback 0x22A
> root@raspcm:~# Write failed: Broken pipe
> 
> (requires /dev/mem without restrictions)
> 
> So this means that it is plld_core related - at least as of now
> and the sdram pll currently derives from PLLD_core.
> 
> @Eric: so please look at the hld (which you have hinted you have
> access to) and come up with something better for sdram
> 
> As far as I can tell at the very least the sdram clock is critical
> and should be marked as such…

So I now got a relatively simple driver that requests:
* the sdram clock
* the ic0 and ic1 register ranges (which afaik are sdram related)
 * exposes those registers read-only via debugfs.

this way the plld_core clock gets enabled and we should be fine.

Device-tree wise it looks like this:
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 2b5cbc6..525f1f2 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -22,6 +22,12 @@
                #address-cells = <1>;
                #size-cells = <1>;

+               memory-controller@7e002000 {
+                       compatible = "brcm,bcm2835-sdram";
+                       reg = <0x7e002000 0x58>, <0x7e002800 0x58>;
+                       clocks = <&clocks BCM2835_CLOCK_SDRAM>;
+               };
+
                timer@7e003000 {
                        compatible = "brcm,bcm2835-system-timer";
                        reg = <0x7e003000 0x1000>;

Note that it looks as if only one bank is ever used on the RPI,
but as I do not know all the details which ones are really used 
I have added both for now.

I guess the other potential missing things are the plla_* related
clocks, which - afaik - shold belong to the arm in some context,
but which are - so far - not claimed, which could result in hickups
unless the videodriver is enabled (and this uses the h264, isp and v3d
clocks)

On a similar front I also have a thermal driver that reads the ts
registers with similar functionality (clock and register wise) - 
it exposes: /sys/class/thermal/thermal_zone0/temp

Martin

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 11.05.2016, at 10:21, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root at raspcm:~# dmesg -C
>>>> root at raspcm:~# modprobe amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root at raspcm:~# rmmod  amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <0000000020201000-0000000020201fff>
>>>> root at raspcm:~# modprobe amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root at raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root at raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x00000296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root at raspcm:~# dmesg -C
>>>> root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>>> modprobe snd-soc-hifiberry-dac
>>>> root at raspcm:~# dmesg
>>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>>> mapping ok
>>>> root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>>> [1] 579
>>>> root at raspcm:~#
>>>> speaker-test 1.0.28
>>>> 
>>>> Playback device is default
>>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>>> Sine wave rate is 440.0000Hz
>>>> Rate set to 44100Hz (requested 44100Hz)
>>>> Buffer size range from 128 to 131072
>>>> Period size range from 64 to 65536
>>>> Using max buffer size 131072
>>>> Periods = 4
>>>> was set period_size = 32768
>>>> was set buffer_size = 131072
>>>> 0 - Front Left
>>>> 1 - Front Right
>>>> 
>>>> root at raspcm:~#
>>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> root at raspcm:~# kill %1
>>>> root at raspcm:~# Time per period = 106.889502
>>>> Timeout, server raspcm not responding.
>>>> 
>>>> You see that plld gets now used and when I kill speaker-test
>>>> the machine crashes again.
>>> 
>>> Just so I can be clear here: What are you using to talk to the Pi?
>>> Builtin USB ethernet?
>> Compute module with USB  Ethernet adapter, but I also got an 
>> enc28j60 connected via spi, but that is not enabled by default
>> 
>>> 
>>>> So this patchset does not really solve any of the problems that
>>>> I have reported either.
>>>> 
>>>> That is why my patchset has taken the "HAND_OFF" approach
>>>> instead (which still just hides some of the issues), but at least
>>>> it does not crash the system on the use of plld and it allows
>>>> for custom parent and mash selection.
>>> 
>>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>>> writing patches using it doesn't make much sense to me.
>> 
>> If it is critical or hands-off does not really make a difference...
>> 
>>>> In reality it would require consumers of the corresponding
>>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>>> clocks are really needed by the firmware - i.e plld.
>>>> 
>>>> Note that the sdram clock is using plld_core parent!
>>>> root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>>> ctl = 0x00004006
>>>> div = 0x00003000
>>>> root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>>> 166533331
>>> 
>>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
>> You are probably right there, but still there must be something
>> hidden that requires plld or plld_per or plld_core, that requires critical.
>> 
>> There must be some reason why it gets set up during the
>> boot process by the firmware.
>> 
> 
> Correction:
> 
> Actually the SDC control register contains some more bits compared
> to most other clock control registers:
> CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)
> 
> See also:
> https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl
> 
> If I poke that register (0x201011a8) from userspace with 0x5a000000
> then the machine crashes as well - so I assume these bits control
> the custom SD-Ram PLL - unfortunately it still does not say which
> PLL is used.
> 
> Eric, you may have access to more data regarding this register -
> maybe we need to hand this register as a special clock?
> and/or expose it as an additional pll?
> 
> Note that I also tried disabling PLLD_CORE and the system crashed as well
> (root at raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
> /dev/mem opened.
> Memory mapped at address 0xb6fdd000.
> Value at address 0x2010110C (0xb6fdd10c): 0x20A
> root at raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
> /dev/mem opened.
> Memory mapped at address 0xb6fe7000.
> Value at address 0x2010110C (0xb6fe710c): 0x20A
> Written 0x5A00022A; readback 0x22A
> root at raspcm:~# Write failed: Broken pipe
> 
> (requires /dev/mem without restrictions)
> 
> So this means that it is plld_core related - at least as of now
> and the sdram pll currently derives from PLLD_core.
> 
> @Eric: so please look at the hld (which you have hinted you have
> access to) and come up with something better for sdram
> 
> As far as I can tell at the very least the sdram clock is critical
> and should be marked as such?

So I now got a relatively simple driver that requests:
* the sdram clock
* the ic0 and ic1 register ranges (which afaik are sdram related)
 * exposes those registers read-only via debugfs.

this way the plld_core clock gets enabled and we should be fine.

Device-tree wise it looks like this:
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 2b5cbc6..525f1f2 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -22,6 +22,12 @@
                #address-cells = <1>;
                #size-cells = <1>;

+               memory-controller at 7e002000 {
+                       compatible = "brcm,bcm2835-sdram";
+                       reg = <0x7e002000 0x58>, <0x7e002800 0x58>;
+                       clocks = <&clocks BCM2835_CLOCK_SDRAM>;
+               };
+
                timer at 7e003000 {
                        compatible = "brcm,bcm2835-system-timer";
                        reg = <0x7e003000 0x1000>;

Note that it looks as if only one bank is ever used on the RPI,
but as I do not know all the details which ones are really used 
I have added both for now.

I guess the other potential missing things are the plla_* related
clocks, which - afaik - shold belong to the arm in some context,
but which are - so far - not claimed, which could result in hickups
unless the videodriver is enabled (and this uses the h264, isp and v3d
clocks)

On a similar front I also have a thermal driver that reads the ts
registers with similar functionality (clock and register wise) - 
it exposes: /sys/class/thermal/thermal_zone0/temp

Martin

Re: [PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Stephen Boyd]

On 05/09, Eric Anholt wrote:
> If the firmware had set up a clock to source from PLLC, go along with
> it.  But if we're looking for a new parent, we don't want to switch it
> to PLLC because the firmware will force PLLC (and thus the AXI bus
> clock) to different frequencies during over-temp/under-voltage,
> without notification to Linux.
> 
> On my system, this moves the Linux-enabled HDMI state machine and DSI1
> escape clock over to plld_per from pllc_per.  EMMC still ends up on
> pllc_per, because the firmware had set it up to use that.

Is it ok for EMMC rate to change with over-temp/under-voltage?
The description makes it sound like PLLC is for clks that want to
run at some system bus rate and they don't care about exact
frequencies.

> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 1091012ecec6..1d8f29ea9f69 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>  	return 0;
>  }
>  
> +static bool
> +bcm2835_clk_is_pllc(struct clk_hw *hw)
> +{
> +	if (!hw)
> +		return false;
> +
> +	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;

This strcmp is not great. Any chance we could look for the parent
by reading the hardware and knowing what bit corresponds to pllc
as a parent? That would be much nicer so that we don't rely on
string comparisons for something the hardware can tell us.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Stephen Boyd]

On 05/09, Eric Anholt wrote:
> If the firmware had set up a clock to source from PLLC, go along with
> it.  But if we're looking for a new parent, we don't want to switch it
> to PLLC because the firmware will force PLLC (and thus the AXI bus
> clock) to different frequencies during over-temp/under-voltage,
> without notification to Linux.
> 
> On my system, this moves the Linux-enabled HDMI state machine and DSI1
> escape clock over to plld_per from pllc_per.  EMMC still ends up on
> pllc_per, because the firmware had set it up to use that.

Is it ok for EMMC rate to change with over-temp/under-voltage?
The description makes it sound like PLLC is for clks that want to
run at some system bus rate and they don't care about exact
frequencies.

> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 1091012ecec6..1d8f29ea9f69 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>  	return 0;
>  }
>  
> +static bool
> +bcm2835_clk_is_pllc(struct clk_hw *hw)
> +{
> +	if (!hw)
> +		return false;
> +
> +	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;

This strcmp is not great. Any chance we could look for the parent
by reading the hardware and knowing what bit corresponds to pllc
as a parent? That would be much nicer so that we don't rely on
string comparisons for something the hardware can tell us.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Eric Anholt]

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

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 05/09, Eric Anholt wrote:
>> If the firmware had set up a clock to source from PLLC, go along with
>> it.  But if we're looking for a new parent, we don't want to switch it
>> to PLLC because the firmware will force PLLC (and thus the AXI bus
>> clock) to different frequencies during over-temp/under-voltage,
>> without notification to Linux.
>> 
>> On my system, this moves the Linux-enabled HDMI state machine and DSI1
>> escape clock over to plld_per from pllc_per.  EMMC still ends up on
>> pllc_per, because the firmware had set it up to use that.
>
> Is it ok for EMMC rate to change with over-temp/under-voltage?
> The description makes it sound like PLLC is for clks that want to
> run at some system bus rate and they don't care about exact
> frequencies.

I'm surprised it's OK for it to change, but the firmware is very
intentionally putting it on PLLC (there's this #define for where most
peripherals go, and EMMC doesn't use it and goes for PLLC instead).  If
we do decide we want to override the firmware, I suspect we'll use
assigned-clock-parents for that.

>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index 1091012ecec6..1d8f29ea9f69 100644
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>>  	return 0;
>>  }
>>  
>> +static bool
>> +bcm2835_clk_is_pllc(struct clk_hw *hw)
>> +{
>> +	if (!hw)
>> +		return false;
>> +
>> +	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;
>
> This strcmp is not great. Any chance we could look for the parent
> by reading the hardware and knowing what bit corresponds to pllc
> as a parent? That would be much nicer so that we don't rely on
> string comparisons for something the hardware can tell us.

We just have the parent index, but which indices are pllc dividers is
different between bcm2835_clock_per_parents and
bcm2835_clock_vpu_parents.  So, I guess it's possible, but seems more
error-prone than the strncmp.

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

[PATCH 3/3] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Eric Anholt]

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 05/09, Eric Anholt wrote:
>> If the firmware had set up a clock to source from PLLC, go along with
>> it.  But if we're looking for a new parent, we don't want to switch it
>> to PLLC because the firmware will force PLLC (and thus the AXI bus
>> clock) to different frequencies during over-temp/under-voltage,
>> without notification to Linux.
>> 
>> On my system, this moves the Linux-enabled HDMI state machine and DSI1
>> escape clock over to plld_per from pllc_per.  EMMC still ends up on
>> pllc_per, because the firmware had set it up to use that.
>
> Is it ok for EMMC rate to change with over-temp/under-voltage?
> The description makes it sound like PLLC is for clks that want to
> run at some system bus rate and they don't care about exact
> frequencies.

I'm surprised it's OK for it to change, but the firmware is very
intentionally putting it on PLLC (there's this #define for where most
peripherals go, and EMMC doesn't use it and goes for PLLC instead).  If
we do decide we want to override the firmware, I suspect we'll use
assigned-clock-parents for that.

>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks")
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index 1091012ecec6..1d8f29ea9f69 100644
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1008,16 +1008,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
>>  	return 0;
>>  }
>>  
>> +static bool
>> +bcm2835_clk_is_pllc(struct clk_hw *hw)
>> +{
>> +	if (!hw)
>> +		return false;
>> +
>> +	return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0;
>
> This strcmp is not great. Any chance we could look for the parent
> by reading the hardware and knowing what bit corresponds to pllc
> as a parent? That would be much nicer so that we don't rely on
> string comparisons for something the hardware can tell us.

We just have the parent index, but which indices are pllc dividers is
different between bcm2835_clock_per_parents and
bcm2835_clock_vpu_parents.  So, I guess it's possible, but seems more
error-prone than the strncmp.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160511/cd7e6df2/attachment-0001.sig>

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

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

>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> and also hsm (probably hardware security module):
>>> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
>>> ctl = 0x000002d6
>>> div = 0x000030e0
>>> root@raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
>>> 163551916
>> 
>> That's the HDMI state machine (there's even a comment saying so),
>> controlled by the vc4 driver.
>> 
>>> So turning plld off stops sdram and hsm - at least that is my
>>> interpretation.
>>> 
>>> This means we need to define a clock property in firmware or
>>> we need a ram node making use of "mmio-sram" maybe?
>>> 
>>> Marking sdram as "critical" or "hand_off" could also solve that
>>> for the moment (but it does not solve all the other hidden
>>> clock dependencies of the firmware)
>> 
>> If there are other hidden dependencies, then we should figure them out.
>
> But strangely the sdram (plus the below) is the only one with
> plld that is enabled (unless it is one of the clocks we have not
> added to the kernel side yet)
>
> Maybe there is something that derives directly from plld_core
> or any of the other plld-dividers?

Nothing I can find.

> Core would indicate anything central to the videocore...
> Anyway both plld_core as well as plld_per ad well as both
> Pll_dsi that are running by default (but I doubt that the
> Dsi would be relevant)
>
> I guess you are in a better situation to figure out which
> hidden HW blocks uses plld...
>
>> 
>>> --- a/drivers/clk/bcm/clk-bcm2835.c
>>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>>> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
>>> clk_desc_array[] = {
>>>                .ctl_reg = CM_SDCCTL,
>>>                .div_reg = CM_SDCDIV,
>>>                .int_bits = 6,
>>> -               .frac_bits = 0),
>>> +               .frac_bits = 0,
>>> +               .flags = CLK_IS_CRITICAL),
>>>        [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>>>                .name = "v3d",
>>>                .ctl_reg = CM_V3DCTL,
>> 
>> The Pi foundation folks believe that the cprman SDRAM clock isn't ever
>> used (there's a separate PLL in the SDRAM controller, and cprman is only
>> intended for unused low-power states), and at least in your sample of
>> the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
> See my comment above - it must be configured for some reason
> during the boot process by the firmware .

You have to write the register to configure the sdram controller's clock
parent.

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

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>> 
>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> and also hsm (probably hardware security module):
>>> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/regdump
>>> ctl = 0x000002d6
>>> div = 0x000030e0
>>> root at raspcm:~# cat /sys/kernel/debug/clk/hsm/clk_rate
>>> 163551916
>> 
>> That's the HDMI state machine (there's even a comment saying so),
>> controlled by the vc4 driver.
>> 
>>> So turning plld off stops sdram and hsm - at least that is my
>>> interpretation.
>>> 
>>> This means we need to define a clock property in firmware or
>>> we need a ram node making use of "mmio-sram" maybe?
>>> 
>>> Marking sdram as "critical" or "hand_off" could also solve that
>>> for the moment (but it does not solve all the other hidden
>>> clock dependencies of the firmware)
>> 
>> If there are other hidden dependencies, then we should figure them out.
>
> But strangely the sdram (plus the below) is the only one with
> plld that is enabled (unless it is one of the clocks we have not
> added to the kernel side yet)
>
> Maybe there is something that derives directly from plld_core
> or any of the other plld-dividers?

Nothing I can find.

> Core would indicate anything central to the videocore...
> Anyway both plld_core as well as plld_per ad well as both
> Pll_dsi that are running by default (but I doubt that the
> Dsi would be relevant)
>
> I guess you are in a better situation to figure out which
> hidden HW blocks uses plld...
>
>> 
>>> --- a/drivers/clk/bcm/clk-bcm2835.c
>>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>>> @@ -1655,7 +1655,8 @@ static const struct bcm2835_clk_desc 
>>> clk_desc_array[] = {
>>>                .ctl_reg = CM_SDCCTL,
>>>                .div_reg = CM_SDCDIV,
>>>                .int_bits = 6,
>>> -               .frac_bits = 0),
>>> +               .frac_bits = 0,
>>> +               .flags = CLK_IS_CRITICAL),
>>>        [BCM2835_CLOCK_V3D]     = REGISTER_VPU_CLK(
>>>                .name = "v3d",
>>>                .ctl_reg = CM_V3DCTL,
>> 
>> The Pi foundation folks believe that the cprman SDRAM clock isn't ever
>> used (there's a separate PLL in the SDRAM controller, and cprman is only
>> intended for unused low-power states), and at least in your sample of
>> the reg, it's not enabled.  Instead of grepping for clk_enable_count, it
> See my comment above - it must be configured for some reason
> during the boot process by the firmware .

You have to write the register to configure the sdram controller's clock
parent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160512/b537a4e4/attachment.sig>

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

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

> On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# rmmod  amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <0000000020201000-0000000020201fff>
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root@raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x00000296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>>> modprobe snd-soc-hifiberry-dac
>>>> root@raspcm:~# dmesg
>>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>>> mapping ok
>>>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>>> [1] 579
>>>> root@raspcm:~#
>>>> speaker-test 1.0.28
>>>> 
>>>> Playback device is default
>>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>>> Sine wave rate is 440.0000Hz
>>>> Rate set to 44100Hz (requested 44100Hz)
>>>> Buffer size range from 128 to 131072
>>>> Period size range from 64 to 65536
>>>> Using max buffer size 131072
>>>> Periods = 4
>>>> was set period_size = 32768
>>>> was set buffer_size = 131072
>>>> 0 - Front Left
>>>> 1 - Front Right
>>>> 
>>>> root@raspcm:~#
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> root@raspcm:~# kill %1
>>>> root@raspcm:~# Time per period = 106.889502
>>>> Timeout, server raspcm not responding.
>>>> 
>>>> You see that plld gets now used and when I kill speaker-test
>>>> the machine crashes again.
>>> 
>>> Just so I can be clear here: What are you using to talk to the Pi?
>>> Builtin USB ethernet?
>> Compute module with USB  Ethernet adapter, but I also got an 
>> enc28j60 connected via spi, but that is not enabled by default
>> 
>>> 
>>>> So this patchset does not really solve any of the problems that
>>>> I have reported either.
>>>> 
>>>> That is why my patchset has taken the "HAND_OFF" approach
>>>> instead (which still just hides some of the issues), but at least
>>>> it does not crash the system on the use of plld and it allows
>>>> for custom parent and mash selection.
>>> 
>>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>>> writing patches using it doesn't make much sense to me.
>> 
>> If it is critical or hands-off does not really make a difference...
>> 
>>>> In reality it would require consumers of the corresponding
>>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>>> clocks are really needed by the firmware - i.e plld.
>>>> 
>>>> Note that the sdram clock is using plld_core parent!
>>>> root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>>> ctl = 0x00004006
>>>> div = 0x00003000
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>>> 166533331
>>> 
>>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
>> You are probably right there, but still there must be something
>> hidden that requires plld or plld_per or plld_core, that requires critical.
>> 
>> There must be some reason why it gets set up during the
>> boot process by the firmware.
>> 
>
> Correction:
>
> Actually the SDC control register contains some more bits compared
> to most other clock control registers:
> CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)
>
> See also:
> https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl
>
> If I poke that register (0x201011a8) from userspace with 0x5a000000
> then the machine crashes as well - so I assume these bits control
> the custom SD-Ram PLL - unfortunately it still does not say which
> PLL is used.

You must not change the CTRL bits while ACCEPT is unset.

> Note that I also tried disabling PLLD_CORE and the system crashed as well
> (root@raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
> /dev/mem opened.
> Memory mapped at address 0xb6fdd000.
> Value at address 0x2010110C (0xb6fdd10c): 0x20A
> root@raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
> /dev/mem opened.
> Memory mapped at address 0xb6fe7000.
> Value at address 0x2010110C (0xb6fe710c): 0x20A
> Written 0x5A00022A; readback 0x22A
> root@raspcm:~# Write failed: Broken pipe

I don't have an answer for this one, and spent a while digging.

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

[PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Eric Anholt]

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

> On 10.05.2016, at 21:58, Martin Sperl <kernel@martin.sperl.org> wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> Martin Sperl <kernel@martin.sperl.org> writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root at raspcm:~# dmesg -C
>>>> root at raspcm:~# modprobe amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root at raspcm:~# rmmod  amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <0000000020201000-0000000020201fff>
>>>> root at raspcm:~# modprobe amba_pl011
>>>> root at raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root at raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root at raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root at raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x00000296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root at raspcm:~# dmesg -C
>>>> root at raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>>> modprobe snd-soc-hifiberry-dac
>>>> root at raspcm:~# dmesg
>>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>>> mapping ok
>>>> root at raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>>> [1] 579
>>>> root at raspcm:~#
>>>> speaker-test 1.0.28
>>>> 
>>>> Playback device is default
>>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>>> Sine wave rate is 440.0000Hz
>>>> Rate set to 44100Hz (requested 44100Hz)
>>>> Buffer size range from 128 to 131072
>>>> Period size range from 64 to 65536
>>>> Using max buffer size 131072
>>>> Periods = 4
>>>> was set period_size = 32768
>>>> was set buffer_size = 131072
>>>> 0 - Front Left
>>>> 1 - Front Right
>>>> 
>>>> root at raspcm:~#
>>>> root at raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pcm/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld/clk_enable_count:1
>>>> /sys/kernel/debug/clk/plld_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> root at raspcm:~# kill %1
>>>> root at raspcm:~# Time per period = 106.889502
>>>> Timeout, server raspcm not responding.
>>>> 
>>>> You see that plld gets now used and when I kill speaker-test
>>>> the machine crashes again.
>>> 
>>> Just so I can be clear here: What are you using to talk to the Pi?
>>> Builtin USB ethernet?
>> Compute module with USB  Ethernet adapter, but I also got an 
>> enc28j60 connected via spi, but that is not enabled by default
>> 
>>> 
>>>> So this patchset does not really solve any of the problems that
>>>> I have reported either.
>>>> 
>>>> That is why my patchset has taken the "HAND_OFF" approach
>>>> instead (which still just hides some of the issues), but at least
>>>> it does not crash the system on the use of plld and it allows
>>>> for custom parent and mash selection.
>>> 
>>> HAND_OFF sure doesn't look like it's landing in the next kernel, so
>>> writing patches using it doesn't make much sense to me.
>> 
>> If it is critical or hands-off does not really make a difference...
>> 
>>>> In reality it would require consumers of the corresponding
>>>> parent clocks in the kernel (arm, ...) and the knowledge which
>>>> clocks are really needed by the firmware - i.e plld.
>>>> 
>>>> Note that the sdram clock is using plld_core parent!
>>>> root at raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
>>>> ctl = 0x00004006
>>>> div = 0x00003000
>>>> root at raspcm:~# cat /sys/kernel/debug/clk/sdram/clk_rate
>>>> 166533331
>>> 
>>> However, it's not enabled, right?  Bit 4 isn't set in the CTL reg.
>> You are probably right there, but still there must be something
>> hidden that requires plld or plld_per or plld_core, that requires critical.
>> 
>> There must be some reason why it gets set up during the
>> boot process by the firmware.
>> 
>
> Correction:
>
> Actually the SDC control register contains some more bits compared
> to most other clock control registers:
> CM_SDCCTL_CTRL (12-15), CM_SDCCTL_ACCPT(16) and CM_SDCCTL_UPDATE(17)
>
> See also:
> https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_sdcctl
>
> If I poke that register (0x201011a8) from userspace with 0x5a000000
> then the machine crashes as well - so I assume these bits control
> the custom SD-Ram PLL - unfortunately it still does not say which
> PLL is used.

You must not change the CTRL bits while ACCEPT is unset.

> Note that I also tried disabling PLLD_CORE and the system crashed as well
> (root at raspcm:~#  peekpoke $[0x20101000 + 0x10c] w
> /dev/mem opened.
> Memory mapped at address 0xb6fdd000.
> Value at address 0x2010110C (0xb6fdd10c): 0x20A
> root at raspcm:~# peekpoke $[0x20101000 + 0x10c] w $[0x5a000000 + 0x20a + 32]
> /dev/mem opened.
> Memory mapped at address 0xb6fe7000.
> Value at address 0x2010110C (0xb6fe710c): 0x20A
> Written 0x5A00022A; readback 0x22A
> root at raspcm:~# Write failed: Broken pipe

I don't have an answer for this one, and spent a while digging.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160512/7be444d5/attachment-0001.sig>