[PATCH 1/2] dt-bindings: display: bridge: icn6211: Add support for external REFCLK

[PATCH 1/2] dt-bindings: display: bridge: icn6211: Add support for external REFCLK

[Marek Vasut]

The ICN6211 is capable of deriving its internal PLL clock from either
MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
Currently supported is only the first option. Document support for
external REFCLK clock input in addition to that.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/bridge/chipone,icn6211.yaml         | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
index 4f0b7c71313c3..18563ebed1a96 100644
--- a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
@@ -24,6 +24,15 @@ properties:
     maxItems: 1
     description: virtual channel number of a DSI peripheral
 
+  clock-names:
+    const: "refclk"
+
+  clocks:
+    maxItems: 1
+    description: |
+        Optional external clock connected to REF_CLK input.
+        The clock rate must be in 10..154 MHz range.
+
   enable-gpios:
     description: Bridge EN pin, chip is reset when EN is low.
 
-- 
2.35.1

[PATCH 2/2] drm: bridge: icn6211: Add support for external REFCLK

[Marek Vasut]

The ICN6211 is capable of deriving its internal PLL clock from either
MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
Currently supported is only the first option. Add support for external
REFCLK clock input in addition to that.

There is little difference between these options, except that in case
of MIPI DSI HS clock input, the HS clock are pre-divided by a fixed /4
divider before being fed to the PLL input, while in case of external
REFCLK, the RECLK clock are fed directly into the PLL input.

Per exceptionally poor documentation, the REFCLK must be in range of
10..154 MHz.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 39 +++++++++++++++++++++---
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 65966f280cf4e..7ee1858bab321 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -11,6 +11,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -151,6 +152,8 @@ struct chipone {
 	struct regulator *vdd1;
 	struct regulator *vdd2;
 	struct regulator *vdd3;
+	struct clk *refclk;
+	unsigned long refclk_rate;
 	bool interface_i2c;
 };
 
@@ -273,7 +276,10 @@ static void chipone_configure_pll(struct chipone *icn,
 	 * It seems the PLL input clock after applying P pre-divider have
 	 * to be lower than 20 MHz.
 	 */
-	fin = icn->dsi->hs_rate / 4; /* in Hz */
+	if (icn->refclk)
+		fin = icn->refclk_rate;
+	else
+		fin = icn->dsi->hs_rate / 4; /* in Hz */
 
 	/* Minimum value of P predivider for PLL input in 5..20 MHz */
 	p_min = clamp(DIV_ROUND_UP(fin, 20000000), 1U, 31U);
@@ -318,16 +324,18 @@ static void chipone_configure_pll(struct chipone *icn,
 	best_p_pot = !(best_p & 1);
 
 	dev_dbg(icn->dev,
-		"PLL: P[3:0]=%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in=%d Hz ; DPI f_out=%d Hz\n",
+		"PLL: P[3:0]=%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in(%s)=%d Hz ; DPI f_out=%d Hz\n",
 		best_p >> best_p_pot, best_p_pot, best_m, best_s + 1,
-		min_delta, fin, (fin * best_m) / (best_p << (best_s + 1)));
+		min_delta, icn->refclk ? "EXT" : "DSI", fin,
+		(fin * best_m) / (best_p << (best_s + 1)));
 
 	ref_div = PLL_REF_DIV_P(best_p >> best_p_pot) | PLL_REF_DIV_S(best_s);
 	if (best_p_pot)	/* Prefer /2 pre-divider */
 		ref_div |= PLL_REF_DIV_Pe;
 
-	/* Clock source selection fixed to MIPI DSI clock lane */
-	chipone_writeb(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+	/* Clock source selection either external clock or MIPI DSI clock lane */
+	chipone_writeb(icn, PLL_CTRL(6),
+		       icn->refclk ? PLL_CTRL_6_EXTERNAL : PLL_CTRL_6_MIPI_CLK);
 	chipone_writeb(icn, PLL_REF_DIV, ref_div);
 	chipone_writeb(icn, PLL_INT(0), best_m);
 }
@@ -463,6 +471,11 @@ static void chipone_atomic_pre_enable(struct drm_bridge *bridge,
 				      "failed to enable VDD3 regulator: %d\n", ret);
 	}
 
+	ret = clk_prepare_enable(icn->refclk);
+	if (ret)
+		DRM_DEV_ERROR(icn->dev,
+			      "failed to enable RECLK clock: %d\n", ret);
+
 	gpiod_set_value(icn->enable_gpio, 1);
 
 	usleep_range(10000, 11000);
@@ -473,6 +486,8 @@ static void chipone_atomic_post_disable(struct drm_bridge *bridge,
 {
 	struct chipone *icn = bridge_to_chipone(bridge);
 
+	clk_disable_unprepare(icn->refclk);
+
 	if (icn->vdd1)
 		regulator_disable(icn->vdd1);
 
@@ -618,6 +633,20 @@ static int chipone_parse_dt(struct chipone *icn)
 	struct device *dev = icn->dev;
 	int ret;
 
+	icn->refclk = devm_clk_get_optional(dev, "refclk");
+	if (IS_ERR(icn->refclk)) {
+		ret = PTR_ERR(icn->refclk);
+		DRM_DEV_ERROR(dev, "failed to get REFCLK clock: %d\n", ret);
+		return ret;
+	} else if (icn->refclk) {
+		icn->refclk_rate = clk_get_rate(icn->refclk);
+		if (icn->refclk_rate < 10000000 || icn->refclk_rate > 154000000) {
+			DRM_DEV_ERROR(dev, "REFCLK out of range: %ld Hz\n",
+				      icn->refclk_rate);
+			return -EINVAL;
+		}
+	}
+
 	icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
 	if (IS_ERR(icn->vdd1)) {
 		ret = PTR_ERR(icn->vdd1);
-- 
2.35.1

Re: [PATCH 2/2] drm: bridge: icn6211: Add support for external REFCLK

[Robert Foss]

Hey Marek,

On Mon, 1 Aug 2022 at 15:18, Marek Vasut <marex@denx.de> wrote:
>
> The ICN6211 is capable of deriving its internal PLL clock from either
> MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
> Currently supported is only the first option. Add support for external
> REFCLK clock input in addition to that.
>
> There is little difference between these options, except that in case
> of MIPI DSI HS clock input, the HS clock are pre-divided by a fixed /4
> divider before being fed to the PLL input, while in case of external
> REFCLK, the RECLK clock are fed directly into the PLL input.
>
> Per exceptionally poor documentation, the REFCLK must be in range of
> 10..154 MHz.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/chipone-icn6211.c | 39 +++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 65966f280cf4e..7ee1858bab321 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> @@ -151,6 +152,8 @@ struct chipone {
>         struct regulator *vdd1;
>         struct regulator *vdd2;
>         struct regulator *vdd3;
> +       struct clk *refclk;
> +       unsigned long refclk_rate;
>         bool interface_i2c;
>  };
>
> @@ -273,7 +276,10 @@ static void chipone_configure_pll(struct chipone *icn,
>          * It seems the PLL input clock after applying P pre-divider have
>          * to be lower than 20 MHz.
>          */
> -       fin = icn->dsi->hs_rate / 4; /* in Hz */
> +       if (icn->refclk)
> +               fin = icn->refclk_rate;
> +       else
> +               fin = icn->dsi->hs_rate / 4; /* in Hz */
>
>         /* Minimum value of P predivider for PLL input in 5..20 MHz */
>         p_min = clamp(DIV_ROUND_UP(fin, 20000000), 1U, 31U);
> @@ -318,16 +324,18 @@ static void chipone_configure_pll(struct chipone *icn,
>         best_p_pot = !(best_p & 1);
>
>         dev_dbg(icn->dev,
> -               "PLL: P[3:0]=%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in=%d Hz ; DPI f_out=%d Hz\n",
> +               "PLL: P[3:0]=%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in(%s)=%d Hz ; DPI f_out=%d Hz\n",
>                 best_p >> best_p_pot, best_p_pot, best_m, best_s + 1,
> -               min_delta, fin, (fin * best_m) / (best_p << (best_s + 1)));
> +               min_delta, icn->refclk ? "EXT" : "DSI", fin,
> +               (fin * best_m) / (best_p << (best_s + 1)));
>
>         ref_div = PLL_REF_DIV_P(best_p >> best_p_pot) | PLL_REF_DIV_S(best_s);
>         if (best_p_pot) /* Prefer /2 pre-divider */
>                 ref_div |= PLL_REF_DIV_Pe;
>
> -       /* Clock source selection fixed to MIPI DSI clock lane */
> -       chipone_writeb(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
> +       /* Clock source selection either external clock or MIPI DSI clock lane */
> +       chipone_writeb(icn, PLL_CTRL(6),
> +                      icn->refclk ? PLL_CTRL_6_EXTERNAL : PLL_CTRL_6_MIPI_CLK);
>         chipone_writeb(icn, PLL_REF_DIV, ref_div);
>         chipone_writeb(icn, PLL_INT(0), best_m);
>  }
> @@ -463,6 +471,11 @@ static void chipone_atomic_pre_enable(struct drm_bridge *bridge,
>                                       "failed to enable VDD3 regulator: %d\n", ret);
>         }
>
> +       ret = clk_prepare_enable(icn->refclk);
> +       if (ret)
> +               DRM_DEV_ERROR(icn->dev,
> +                             "failed to enable RECLK clock: %d\n", ret);
> +
>         gpiod_set_value(icn->enable_gpio, 1);
>
>         usleep_range(10000, 11000);
> @@ -473,6 +486,8 @@ static void chipone_atomic_post_disable(struct drm_bridge *bridge,
>  {
>         struct chipone *icn = bridge_to_chipone(bridge);
>
> +       clk_disable_unprepare(icn->refclk);
> +
>         if (icn->vdd1)
>                 regulator_disable(icn->vdd1);
>
> @@ -618,6 +633,20 @@ static int chipone_parse_dt(struct chipone *icn)
>         struct device *dev = icn->dev;
>         int ret;
>
> +       icn->refclk = devm_clk_get_optional(dev, "refclk");
> +       if (IS_ERR(icn->refclk)) {
> +               ret = PTR_ERR(icn->refclk);
> +               DRM_DEV_ERROR(dev, "failed to get REFCLK clock: %d\n", ret);
> +               return ret;
> +       } else if (icn->refclk) {
> +               icn->refclk_rate = clk_get_rate(icn->refclk);
> +               if (icn->refclk_rate < 10000000 || icn->refclk_rate > 154000000) {
> +                       DRM_DEV_ERROR(dev, "REFCLK out of range: %ld Hz\n",
> +                                     icn->refclk_rate);
> +                       return -EINVAL;
> +               }
> +       }
> +
>         icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>         if (IS_ERR(icn->vdd1)) {
>                 ret = PTR_ERR(icn->vdd1);
> --
> 2.35.1
>

This patch looks good to me, but it doesn't apply on drm-misc-next. Do
you mind re-spinning it?

Rob.

Re: [PATCH 2/2] drm: bridge: icn6211: Add support for external REFCLK

[Marek Vasut]

On 8/29/22 16:54, Robert Foss wrote:
> Hey Marek,

Hi,

> On Mon, 1 Aug 2022 at 15:18, Marek Vasut <marex@denx.de> wrote:
>>
>> The ICN6211 is capable of deriving its internal PLL clock from either
>> MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
>> Currently supported is only the first option. Add support for external
>> REFCLK clock input in addition to that.
>>
>> There is little difference between these options, except that in case
>> of MIPI DSI HS clock input, the HS clock are pre-divided by a fixed /4
>> divider before being fed to the PLL input, while in case of external
>> REFCLK, the RECLK clock are fed directly into the PLL input.
>>
>> Per exceptionally poor documentation, the REFCLK must be in range of
>> 10..154 MHz.

[...]

> This patch looks good to me, but it doesn't apply on drm-misc-next. Do
> you mind re-spinning it?

I already see this patch in drm-misc-next, so it was applied already:

378e0f9f0b3e0 ("drm: bridge: icn6211: Add support for external REFCLK")

I think all is good ?

Re: [PATCH 1/2] dt-bindings: display: bridge: icn6211: Add support for external REFCLK

[Rob Herring]

On Mon, Aug 01, 2022 at 03:17:46PM +0200, Marek Vasut wrote:
> The ICN6211 is capable of deriving its internal PLL clock from either
> MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
> Currently supported is only the first option. Document support for
> external REFCLK clock input in addition to that.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/bridge/chipone,icn6211.yaml         | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
> index 4f0b7c71313c3..18563ebed1a96 100644
> --- a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
> @@ -24,6 +24,15 @@ properties:
>      maxItems: 1
>      description: virtual channel number of a DSI peripheral
>  
> +  clock-names:
> +    const: "refclk"

Drop quotes.

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] dt-bindings: display: bridge: icn6211: Add support for external REFCLK

[Linus Walleij]

On Mon, Aug 1, 2022 at 3:18 PM Marek Vasut <marex@denx.de> wrote:

> The ICN6211 is capable of deriving its internal PLL clock from either
> MIPI DSI HS clock, external REFCLK clock, or even internal oscillator.
> Currently supported is only the first option. Document support for
> external REFCLK clock input in addition to that.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: devicetree@vger.kernel.org

Fixed up Rob's comment and applied both patches.

Yours,
Linus Walleij