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

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

[Eric Anholt]

I figured out another critical clock (patch 3), but didn't use the
CLK_IS_CRITICAL flag since I want to just protect whatever clock
happens to be the parent (there are #ifdefs in the firmware indicating
that they've experimented with using different clocks as the parent).

I think these fixes are all suitable for 4.7.

Eric Anholt (4):
  clk: bcm2835: Mark the VPU clock as critical
  clk: bcm2835: Mark GPIO clocks enabled at boot as critical
  clk: bcm2835: Mark the CM SDRAM clock's parent as critical
  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

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

-- 
2.8.0.rc3

[PATCH v3 1/4] 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 v3 2/4] 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>
---

v3: Only mark critical if it's on at boot.

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

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index d9db03cb3fd8..400615baea97 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1239,6 +1239,12 @@ static struct clk *bcm2835_register_clock(struct bcm2835_cprman *cprman,
 	} else {
 		init.ops = &bcm2835_clock_clk_ops;
 		init.flags |= CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
+
+		/* If the clock wasn't actually enabled at boot, it's not
+		 * critical.
+		 */
+		if (!(cprman_read(cprman, data->ctl_reg) & CM_ENABLE))
+			init.flags &= ~CLK_IS_CRITICAL;
 	}
 
 	clock = devm_kzalloc(cprman->dev, sizeof(*clock), GFP_KERNEL);
@@ -1708,13 +1714,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 v3 3/4] clk: bcm2835: Mark the CM SDRAM clock's parent as critical

[Eric Anholt]

While the SDRAM is being driven by its dedicated PLL most of the time,
there is a little loop running in the firmware that periodically turns
on the CM SDRAM clock (using its pre-initialized parent) and switches
SDRAM to using the CM clock to do PVT recalibration.

This avoids system hangs if we choose SDRAM's parent for some other
clock, then disable that clock.

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

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 400615baea97..c6420b36cbcb 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -36,6 +36,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
+#include <linux/clk.h>
 #include <linux/clk/bcm2835.h>
 #include <linux/debugfs.h>
 #include <linux/module.h>
@@ -1801,6 +1802,25 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
 		.ctl_reg = CM_PERIICTL),
 };
 
+/*
+ * Permanently take a reference on the parent of the SDRAM clock.
+ *
+ * While the SDRAM is being driven by its dedicated PLL most of the
+ * time, there is a little loop running in the firmware that
+ * periodically switches the SDRAM to using our CM clock to do PVT
+ * recalibration, with the assumption that the previously configured
+ * SDRAM parent is still enabled and running.
+ */
+static int bcm2835_mark_sdc_parent_critical(struct clk *sdc)
+{
+	struct clk *parent = clk_get_parent(sdc);
+
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	return clk_prepare_enable(parent);
+}
+
 static int bcm2835_clk_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1810,6 +1830,7 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
 	const struct bcm2835_clk_desc *desc;
 	const size_t asize = ARRAY_SIZE(clk_desc_array);
 	size_t i;
+	int ret;
 
 	cprman = devm_kzalloc(dev,
 			      sizeof(*cprman) + asize * sizeof(*clks),
@@ -1840,6 +1861,10 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
 			clks[i] = desc->clk_register(cprman, desc->data);
 	}
 
+	ret = bcm2835_mark_sdc_parent_critical(clks[BCM2835_CLOCK_SDRAM]);
+	if (ret)
+		return ret;
+
 	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
 				   &cprman->onecell);
 }
-- 
2.8.0.rc3

[PATCH v3 4/4] 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")
---
 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 c6420b36cbcb..e8a9646afd6d 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1009,16 +1009,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
 	 */
@@ -1026,6 +1038,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 v3 0/4] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 01.06.2016, at 21:05, Eric Anholt <eric@anholt.net> wrote:
> 
> I figured out another critical clock (patch 3), but didn't use the
> CLK_IS_CRITICAL flag since I want to just protect whatever clock
> happens to be the parent (there are #ifdefs in the firmware indicating
> that they've experimented with using different clocks as the parent).
> 
> I think these fixes are all suitable for 4.7.
> 
> Eric Anholt (4):
>  clk: bcm2835: Mark the VPU clock as critical
>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical
>  clk: bcm2835: Mark the CM SDRAM clock's parent as critical
>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
> 
> drivers/clk/bcm/clk-bcm2835.c | 63 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 2 deletions(-)

Whole series:
Acked-by: Martin Sperl <kernel@martin.sperl.org>

Note that these patches are also seeing more testing downstream in 4.7
and there have been no hiccups seen either. Clock selection is working
as expected for I2S as well.

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

[Stephen Boyd]

On 09/07, Martin Sperl wrote:
> 
> > On 01.06.2016, at 21:05, Eric Anholt <eric@anholt.net> wrote:
> > 
> > I figured out another critical clock (patch 3), but didn't use the
> > CLK_IS_CRITICAL flag since I want to just protect whatever clock
> > happens to be the parent (there are #ifdefs in the firmware indicating
> > that they've experimented with using different clocks as the parent).
> > 
> > I think these fixes are all suitable for 4.7.
> > 
> > Eric Anholt (4):
> >  clk: bcm2835: Mark the VPU clock as critical
> >  clk: bcm2835: Mark GPIO clocks enabled at boot as critical
> >  clk: bcm2835: Mark the CM SDRAM clock's parent as critical
> >  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
> > 
> > drivers/clk/bcm/clk-bcm2835.c | 63 +++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 61 insertions(+), 2 deletions(-)
> 
> Whole series:
> Acked-by: Martin Sperl <kernel@martin.sperl.org>
> 
> Note that these patches are also seeing more testing downstream in 4.7
> and there have been no hiccups seen either. Clock selection is working
> as expected for I2S as well.
> 
> 

Ok. Applied all of them to clk-next

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