[PATCH v2] drm/pl111: Register the clock divider and use it.

[PATCH v2] drm/pl111: Register the clock divider and use it.

[Eric Anholt]

This is required for the panel to work on bcm911360, where CLCDCLK is
the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
CLCDCLK, for platforms that have a settable rate on that one.

v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
    COMMON_CLK.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/pl111/Kconfig         |   1 +
 drivers/gpu/drm/pl111/pl111_display.c | 162 ++++++++++++++++++++++++++++++----
 drivers/gpu/drm/pl111/pl111_drm.h     |   8 ++
 drivers/gpu/drm/pl111/pl111_drv.c     |  11 +--
 include/linux/amba/clcd-regs.h        |   5 ++
 5 files changed, 163 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
index ede49efd531f..309f4fd52de7 100644
--- a/drivers/gpu/drm/pl111/Kconfig
+++ b/drivers/gpu/drm/pl111/Kconfig
@@ -2,6 +2,7 @@ config DRM_PL111
 	tristate "DRM Support for PL111 CLCD Controller"
 	depends on DRM
 	depends on ARM || ARM64 || COMPILE_TEST
+	depends on COMMON_CLK
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_GEM_CMA_HELPER
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 39a5c33bce7d..2d924a6bf43c 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -108,7 +108,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
 	u32 cntl;
 	u32 ppl, hsw, hfp, hbp;
 	u32 lpp, vsw, vfp, vbp;
-	u32 cpl;
+	u32 cpl, tim2;
 	int ret;
 
 	ret = clk_set_rate(priv->clk, mode->clock * 1000);
@@ -142,20 +142,28 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
 	       (vfp << 16) |
 	       (vbp << 24),
 	       priv->regs + CLCD_TIM1);
-	/* XXX: We currently always use CLCDCLK with no divisor.  We
-	 * could probably reduce power consumption by using HCLK
-	 * (apb_pclk) with a divisor when it gets us near our target
-	 * pixel clock.
-	 */
-	writel(((mode->flags & DRM_MODE_FLAG_NHSYNC) ? TIM2_IHS : 0) |
-	       ((mode->flags & DRM_MODE_FLAG_NVSYNC) ? TIM2_IVS : 0) |
-	       ((connector->display_info.bus_flags &
-		 DRM_BUS_FLAG_DE_LOW) ? TIM2_IOE : 0) |
-	       ((connector->display_info.bus_flags &
-		 DRM_BUS_FLAG_PIXDATA_NEGEDGE) ? TIM2_IPC : 0) |
-	       TIM2_BCD |
-	       (cpl << 16),
-	       priv->regs + CLCD_TIM2);
+
+	spin_lock(&priv->tim2_lock);
+
+	tim2 = readl(priv->regs + CLCD_TIM2);
+	tim2 &= (TIM2_BCD | TIM2_PCD_LO_MASK | TIM2_PCD_HI_MASK);
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		tim2 |= TIM2_IHS;
+
+	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+		tim2 |= TIM2_IVS;
+
+	if (connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW)
+		tim2 |= TIM2_IOE;
+
+	if (connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
+		tim2 |= TIM2_IPC;
+
+	tim2 |= cpl << 16;
+	writel(tim2, priv->regs + CLCD_TIM2);
+	spin_unlock(&priv->tim2_lock);
+
 	writel(0, priv->regs + CLCD_TIM3);
 
 	drm_panel_prepare(priv->connector.panel);
@@ -288,6 +296,126 @@ const struct drm_simple_display_pipe_funcs pl111_display_funcs = {
 	.prepare_fb = pl111_display_prepare_fb,
 };
 
+static int pl111_clk_div_choose_div(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *prate, bool set_parent)
+{
+	int best_div = 1, div;
+	struct clk_hw *parent = clk_hw_get_parent(hw);
+	unsigned long best_prate = 0;
+	unsigned long best_diff = ~0ul;
+	int max_div = (1 << (TIM2_PCD_LO_BITS + TIM2_PCD_HI_BITS)) - 1;
+
+	for (div = 1; div < max_div; div++) {
+		unsigned long this_prate, div_rate, diff;
+
+		if (set_parent)
+			this_prate = clk_hw_round_rate(parent, rate * div);
+		else
+			this_prate = *prate;
+		div_rate = DIV_ROUND_UP_ULL(this_prate, div);
+		diff = abs(rate - div_rate);
+
+		if (diff < best_diff) {
+			best_div = div;
+			best_diff = diff;
+			best_prate = this_prate;
+		}
+	}
+
+	*prate = best_prate;
+	return best_div;
+}
+
+static long pl111_clk_div_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *prate)
+{
+	int div = pl111_clk_div_choose_div(hw, rate, prate, true);
+
+	return DIV_ROUND_UP_ULL(*prate, div);
+}
+
+static unsigned long pl111_clk_div_recalc_rate(struct clk_hw *hw,
+					       unsigned long prate)
+{
+	struct pl111_drm_dev_private *priv =
+		container_of(hw, struct pl111_drm_dev_private, clk_div);
+	u32 tim2 = readl(priv->regs + CLCD_TIM2);
+	int div;
+
+	if (tim2 & TIM2_BCD)
+		return prate;
+
+	div = tim2 & TIM2_PCD_LO_MASK;
+	div |= (tim2 & TIM2_PCD_HI_MASK) >>
+		(TIM2_PCD_HI_SHIFT - TIM2_PCD_LO_BITS);
+	div += 2;
+
+	return DIV_ROUND_UP_ULL(prate, div);
+}
+
+static int pl111_clk_div_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long prate)
+{
+	struct pl111_drm_dev_private *priv =
+		container_of(hw, struct pl111_drm_dev_private, clk_div);
+	int div = pl111_clk_div_choose_div(hw, rate, &prate, false);
+	u32 tim2;
+
+	spin_lock(&priv->tim2_lock);
+	tim2 = readl(priv->regs + CLCD_TIM2);
+	tim2 &= ~(TIM2_BCD | TIM2_PCD_LO_MASK | TIM2_PCD_HI_MASK);
+
+	if (div == 1) {
+		tim2 |= TIM2_BCD;
+	} else {
+		div -= 2;
+		tim2 |= div & TIM2_PCD_LO_MASK;
+		tim2 |= (div >> TIM2_PCD_LO_BITS) << TIM2_PCD_HI_SHIFT;
+	}
+
+	writel(tim2, priv->regs + CLCD_TIM2);
+	spin_unlock(&priv->tim2_lock);
+
+	return 0;
+}
+
+const struct clk_ops pl111_clk_div_ops = {
+	.recalc_rate = pl111_clk_div_recalc_rate,
+	.round_rate = pl111_clk_div_round_rate,
+	.set_rate = pl111_clk_div_set_rate,
+};
+
+static int
+pl111_init_clock_divider(struct drm_device *drm)
+{
+	struct pl111_drm_dev_private *priv = drm->dev_private;
+	struct clk *parent = devm_clk_get(drm->dev, "clcdclk");
+	struct clk_hw *div = &priv->clk_div;
+	const char *parent_name;
+	struct clk_init_data init = {
+		.name = "pl111_div",
+		.ops = &pl111_clk_div_ops,
+		.parent_names = &parent_name,
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	};
+	int ret;
+
+	if (IS_ERR(parent)) {
+		dev_err(drm->dev, "CLCD: unable to get clcdclk.\n");
+		return PTR_ERR(parent);
+	}
+	parent_name = __clk_get_name(parent);
+
+	spin_lock_init(&priv->tim2_lock);
+	div->init = &init;
+
+	ret = devm_clk_hw_register(drm->dev, div);
+
+	priv->clk = div->clk;
+	return ret;
+}
+
 int pl111_display_init(struct drm_device *drm)
 {
 	struct pl111_drm_dev_private *priv = drm->dev_private;
@@ -333,6 +461,10 @@ int pl111_display_init(struct drm_device *drm)
 		return -EINVAL;
 	}
 
+	ret = pl111_init_clock_divider(drm);
+	if (ret)
+		return ret;
+
 	ret = drm_simple_display_pipe_init(drm, &priv->pipe,
 					   &pl111_display_funcs,
 					   formats, ARRAY_SIZE(formats),
diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index f381593921b7..4162c6aa5dbb 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -21,6 +21,7 @@
 
 #include <drm/drm_gem.h>
 #include <drm/drm_simple_kms_helper.h>
+#include <linux/clk-provider.h>
 
 #define CLCD_IRQ_NEXTBASE_UPDATE BIT(2)
 
@@ -37,7 +38,14 @@ struct pl111_drm_dev_private {
 	struct drm_fbdev_cma *fbdev;
 
 	void *regs;
+	/* The pixel clock (a reference to our clock divider off of CLCDCLK). */
 	struct clk *clk;
+	/* pl111's internal clock divider. */
+	struct clk_hw clk_div;
+	/* Lock to sync access to CLCD_TIM2 between the common clock
+	 * subsystem and pl111_display_enable().
+	 */
+	spinlock_t tim2_lock;
 };
 
 #define to_pl111_connector(x) \
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 936403f65508..9d1467492cb9 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -50,8 +50,8 @@
  * - Read back hardware state at boot to skip reprogramming the
  *   hardware when doing a no-op modeset.
  *
- * - Use the internal clock divisor to reduce power consumption by
- *   using HCLK (apb_pclk) when appropriate.
+ * - Use the CLKSEL bit to support switching between the two external
+ *   clock parents.
  */
 
 #include <linux/amba/bus.h>
@@ -195,13 +195,6 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	priv->drm = drm;
 	drm->dev_private = priv;
 
-	priv->clk = devm_clk_get(dev, "clcdclk");
-	if (IS_ERR(priv->clk)) {
-		dev_err(dev, "CLCD: unable to get clk.\n");
-		ret = PTR_ERR(priv->clk);
-		goto dev_unref;
-	}
-
 	priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
 	if (!priv->regs) {
 		dev_err(dev, "%s failed mmio\n", __func__);
diff --git a/include/linux/amba/clcd-regs.h b/include/linux/amba/clcd-regs.h
index 69c0e2143003..516a6fda83c5 100644
--- a/include/linux/amba/clcd-regs.h
+++ b/include/linux/amba/clcd-regs.h
@@ -39,12 +39,17 @@
 #define CLCD_PALL 		0x00000200
 #define CLCD_PALETTE		0x00000200
 
+#define TIM2_PCD_LO_MASK	GENMASK(4, 0)
+#define TIM2_PCD_LO_BITS	5
 #define TIM2_CLKSEL		(1 << 5)
 #define TIM2_IVS		(1 << 11)
 #define TIM2_IHS		(1 << 12)
 #define TIM2_IPC		(1 << 13)
 #define TIM2_IOE		(1 << 14)
 #define TIM2_BCD		(1 << 26)
+#define TIM2_PCD_HI_MASK	GENMASK(31, 27)
+#define TIM2_PCD_HI_BITS	5
+#define TIM2_PCD_HI_SHIFT	27
 
 #define CNTL_LCDEN		(1 << 0)
 #define CNTL_LCDBPP1		(0 << 1)
-- 
2.11.0

Re: [PATCH v2] drm/pl111: Register the clock divider and use it.

[Linus Walleij]

On Mon, May 8, 2017 at 9:33 PM, Eric Anholt <eric@anholt.net> wrote:

> This is required for the panel to work on bcm911360, where CLCDCLK is
> the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
> CLCDCLK, for platforms that have a settable rate on that one.
>
> v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
>     COMMON_CLK.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2] drm/pl111: Register the clock divider and use it.

[Eric Anholt]

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

Linus Walleij <linus.walleij@linaro.org> writes:

> On Mon, May 8, 2017 at 9:33 PM, Eric Anholt <eric@anholt.net> wrote:
>
>> This is required for the panel to work on bcm911360, where CLCDCLK is
>> the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
>> CLCDCLK, for platforms that have a settable rate on that one.
>>
>> v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
>>     COMMON_CLK.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks.  Waiting on an ack from clock folks, then we'll be ready to go,
I think.

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

Re: [PATCH v2] drm/pl111: Register the clock divider and use it.

[Linus Walleij]

On Tue, May 9, 2017 at 8:18 PM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> On Mon, May 8, 2017 at 9:33 PM, Eric Anholt <eric@anholt.net> wrote:
>>
>>> This is required for the panel to work on bcm911360, where CLCDCLK is
>>> the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
>>> CLCDCLK, for platforms that have a settable rate on that one.
>>>
>>> v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
>>>     COMMON_CLK.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thanks.  Waiting on an ack from clock folks, then we'll be ready to go,
> I think.

Mike/Stephen?

It would be nice to have this queued in -next quite early in the v4.13
development cycle, so I can start looking into migrating the rest of the
old fbdev users to this.

BCM911360 seems like some oddity, there is not even a pictur of
it online, but I guess it's a cool little gadget :)

Yours,
Linus Walleij

Re: [PATCH v2] drm/pl111: Register the clock divider and use it.

[Stephen Boyd]

On 05/08, Eric Anholt wrote:
> This is required for the panel to work on bcm911360, where CLCDCLK is
> the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
> CLCDCLK, for platforms that have a settable rate on that one.
> 
> v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
>     COMMON_CLK.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

One minor comment below

> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 39a5c33bce7d..2d924a6bf43c 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -288,6 +296,126 @@ const struct drm_simple_display_pipe_funcs pl111_display_funcs = {
[...]
> +
> +	return 0;
> +}
> +
> +const struct clk_ops pl111_clk_div_ops = {

static?

> +	.recalc_rate = pl111_clk_div_recalc_rate,
> +	.round_rate = pl111_clk_div_round_rate,
> +	.set_rate = pl111_clk_div_set_rate,
> +};
> +
> +static int

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

Re: [PATCH v2] drm/pl111: Register the clock divider and use it.

[Eric Anholt]

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

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 05/08, Eric Anholt wrote:
>> This is required for the panel to work on bcm911360, where CLCDCLK is
>> the fixed 200Mhz AXI41 clock.  The rate set is still passed up to the
>> CLCDCLK, for platforms that have a settable rate on that one.
>> 
>> v2: Set SET_RATE_PARENT (caught by Linus Walleij), depend on
>>     COMMON_CLK.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>
> One minor comment below
>
>> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
>> index 39a5c33bce7d..2d924a6bf43c 100644
>> --- a/drivers/gpu/drm/pl111/pl111_display.c
>> +++ b/drivers/gpu/drm/pl111/pl111_display.c
>> @@ -288,6 +296,126 @@ const struct drm_simple_display_pipe_funcs pl111_display_funcs = {
> [...]
>> +
>> +	return 0;
>> +}
>> +
>> +const struct clk_ops pl111_clk_div_ops = {
>
> static?

Fixed, thanks.

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