[PATCH 0/4] host1x: mipi: Some patches to improve d-phy calibration

[PATCH 0/4] host1x: mipi: Some patches to improve d-phy calibration

[Sean Paul]

A few patches to make mipi d-phy calibration match the procedure in the TRM,
as well as adding the option to enable a parent clock during calibration.

Sean Paul (4):
  host1x: mipi: Add new parent clock for mipi calibration
  host1x: mipi: Preserve the contents of MIPI_CAL_CTRL
  host1x: mipi: Include clock lanes in mipi calibrate
  host1x: mipi: Set MIPI_CAL_BIAS_PAD_CFG1 register

 .../bindings/mipi/nvidia,tegra114-mipi.txt         |  12 ++-
 drivers/gpu/host1x/mipi.c                          | 120 +++++++++++++++++----
 2 files changed, 105 insertions(+), 27 deletions(-)

-- 
2.0.0

[PATCH 1/4] host1x: mipi: Add new parent clock for mipi calibration

[Sean Paul]

This patch adds a new parent clock to enable/disable the 72MHz
clock required for mipi calibration.

Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 .../bindings/mipi/nvidia,tegra114-mipi.txt         | 12 ++++--
 drivers/gpu/host1x/mipi.c                          | 44 ++++++++++++++++++----
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/mipi/nvidia,tegra114-mipi.txt b/Documentation/devicetree/bindings/mipi/nvidia,tegra114-mipi.txt
index e4a25ce..6e1aad0 100644
--- a/Documentation/devicetree/bindings/mipi/nvidia,tegra114-mipi.txt
+++ b/Documentation/devicetree/bindings/mipi/nvidia,tegra114-mipi.txt
@@ -5,8 +5,11 @@ Required properties:
 - reg: Physical base address and length of the controller's registers.
 - clocks: Must contain an entry for each entry in clock-names.
   See ../clocks/clock-bindings.txt for details.
-- clock-names: Must include the following entries:
-  - mipi-cal
+- clock-names: Include the following entries:
+  - REQUIRED CLOCKS:
+    - mipi-cal
+  - OPTIONAL CLOCKS:
+    - parent
 - #nvidia,mipi-calibrate-cells: Should be 1. The cell is a bitmask of the pads
   that need to be calibrated for a given device.
 
@@ -19,8 +22,9 @@ Example:
 	mipi: mipi@700e3000 {
 		compatible = "nvidia,tegra114-mipi";
 		reg = <0x700e3000 0x100>;
-		clocks = <&tegra_car TEGRA114_CLK_MIPI_CAL>;
-		clock-names = "mipi-cal";
+		clocks = <&tegra_car TEGRA114_CLK_MIPI_CAL>,
+			 <&tegra_car TEGRA114_CLK_PLL_P_OUT3>;
+		clock-names = "mipi-cal", "parent";
 		#nvidia,mipi-calibrate-cells = <1>;
 	};
 
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 9882ea1..4dd91fd 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -80,7 +80,8 @@ static const struct module {
 struct tegra_mipi {
 	void __iomem *regs;
 	struct mutex lock;
-	struct clk *clk;
+	struct clk *clk_parent;
+	struct clk *clk_mipi_cal;
 };
 
 struct tegra_mipi_device {
@@ -181,10 +182,16 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	unsigned int i;
 	int err;
 
-	err = clk_enable(device->mipi->clk);
+	err = clk_enable(device->mipi->clk_mipi_cal);
 	if (err < 0)
 		return err;
 
+	if (device->mipi->clk_parent) {
+		err = clk_enable(device->mipi->clk_parent);
+		if (err < 0)
+			goto out_clk_mipi_cal;
+	}
+
 	mutex_lock(&device->mipi->lock);
 
 	value = tegra_mipi_readl(device->mipi, MIPI_CAL_BIAS_PAD_CFG0);
@@ -213,7 +220,12 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	err = tegra_mipi_wait(device->mipi);
 
 	mutex_unlock(&device->mipi->lock);
-	clk_disable(device->mipi->clk);
+
+	if (device->mipi->clk_parent)
+		clk_disable(device->mipi->clk_parent);
+
+out_clk_mipi_cal:
+	clk_disable(device->mipi->clk_mipi_cal);
 
 	return err;
 }
@@ -236,26 +248,42 @@ static int tegra_mipi_probe(struct platform_device *pdev)
 
 	mutex_init(&mipi->lock);
 
-	mipi->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(mipi->clk)) {
+	mipi->clk_mipi_cal = devm_clk_get(&pdev->dev, "mipi-cal");
+	if (IS_ERR(mipi->clk_mipi_cal)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
-		return PTR_ERR(mipi->clk);
+		return PTR_ERR(mipi->clk_mipi_cal);
 	}
 
-	err = clk_prepare(mipi->clk);
+	mipi->clk_parent = devm_clk_get(&pdev->dev, "parent");
+	if (IS_ERR(mipi->clk_parent))
+		mipi->clk_parent = NULL;
+
+	err = clk_prepare(mipi->clk_mipi_cal);
 	if (err < 0)
 		return err;
 
+	if (mipi->clk_parent) {
+		err = clk_prepare(mipi->clk_parent);
+		if (err < 0)
+			goto err;
+	}
+
 	platform_set_drvdata(pdev, mipi);
 
 	return 0;
+err:
+	clk_unprepare(mipi->clk_mipi_cal);
+
+	return err;
 }
 
 static int tegra_mipi_remove(struct platform_device *pdev)
 {
 	struct tegra_mipi *mipi = platform_get_drvdata(pdev);
 
-	clk_unprepare(mipi->clk);
+	if (mipi->clk_parent)
+		clk_unprepare(mipi->clk_parent);
+	clk_unprepare(mipi->clk_mipi_cal);
 
 	return 0;
 }
-- 
2.0.0

[PATCH 2/4] host1x: mipi: Preserve the contents of MIPI_CAL_CTRL

[Sean Paul]

Preserve the contents of MIPI_CAL_CTRL when starting mipi
calibration.

Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/gpu/host1x/mipi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 4dd91fd..0af2892 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -215,7 +215,9 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 		tegra_mipi_writel(device->mipi, value, modules[i].reg);
 	}
 
-	tegra_mipi_writel(device->mipi, MIPI_CAL_CTRL_START, MIPI_CAL_CTRL);
+	value = tegra_mipi_readl(device->mipi, MIPI_CAL_CTRL);
+	value |= MIPI_CAL_CTRL_START;
+	tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
 
 	err = tegra_mipi_wait(device->mipi);
 
-- 
2.0.0

[PATCH 3/4] host1x: mipi: Include clock lanes in mipi calibrate

[Sean Paul]

When calibrating the mipi phy, also include the clock lanes
in the calibration.

Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/gpu/host1x/mipi.c | 70 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 0af2892..80578dc 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -49,10 +49,18 @@
 #define MIPI_CAL_CONFIG_DSIC		0x10
 #define MIPI_CAL_CONFIG_DSID		0x11
 
+#define MIPI_CAL_CONFIG_DSIAB_CLK	0x19
+#define MIPI_CAL_CONFIG_DSICD_CLK	0x1a
+#define MIPI_CAL_CONFIG_CSIAB_CLK	0x1b
+#define MIPI_CAL_CONFIG_CSICD_CLK	0x1c
+#define MIPI_CAL_CONFIG_CSIE_CLK	0x1d
+
 #define MIPI_CAL_CONFIG_SELECT		(1 << 21)
 #define MIPI_CAL_CONFIG_HSPDOS(x)	(((x) & 0x1f) << 16)
 #define MIPI_CAL_CONFIG_HSPUOS(x)	(((x) & 0x1f) <<  8)
 #define MIPI_CAL_CONFIG_TERMOS(x)	(((x) & 0x1f) <<  0)
+#define MIPI_CAL_CONFIG_HSCLKPDOSD(x)	(((x) & 0x1f) << 8)
+#define MIPI_CAL_CONFIG_HSCLKPUOSD(x)	(((x) & 0x1f) <<  0)
 
 #define MIPI_CAL_BIAS_PAD_CFG0		0x16
 #define MIPI_CAL_BIAS_PAD_PDVCLAMP	(1 << 1)
@@ -64,17 +72,45 @@
 #define MIPI_CAL_BIAS_PAD_PDVREG	(1 << 1)
 
 static const struct module {
-	unsigned long reg;
+	unsigned long data_reg;
+	unsigned long clk_reg;
 } modules[] = {
-	{ .reg = MIPI_CAL_CONFIG_CSIA },
-	{ .reg = MIPI_CAL_CONFIG_CSIB },
-	{ .reg = MIPI_CAL_CONFIG_CSIC },
-	{ .reg = MIPI_CAL_CONFIG_CSID },
-	{ .reg = MIPI_CAL_CONFIG_CSIE },
-	{ .reg = MIPI_CAL_CONFIG_DSIA },
-	{ .reg = MIPI_CAL_CONFIG_DSIB },
-	{ .reg = MIPI_CAL_CONFIG_DSIC },
-	{ .reg = MIPI_CAL_CONFIG_DSID },
+	{
+		.data_reg = MIPI_CAL_CONFIG_CSIA,
+		.clk_reg = MIPI_CAL_CONFIG_CSIAB_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_CSIB,
+		.clk_reg = MIPI_CAL_CONFIG_CSIAB_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_CSIC,
+		.clk_reg = MIPI_CAL_CONFIG_CSICD_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_CSID,
+		.clk_reg = MIPI_CAL_CONFIG_CSICD_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_CSIE,
+		.clk_reg = MIPI_CAL_CONFIG_CSIE_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_DSIA,
+		.clk_reg = MIPI_CAL_CONFIG_DSIAB_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_DSIB,
+		.clk_reg = MIPI_CAL_CONFIG_DSIAB_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_DSIC,
+		.clk_reg = MIPI_CAL_CONFIG_DSICD_CLK
+	},
+	{
+		.data_reg = MIPI_CAL_CONFIG_DSID,
+		.clk_reg = MIPI_CAL_CONFIG_DSICD_CLK
+	},
 };
 
 struct tegra_mipi {
@@ -178,7 +214,7 @@ static int tegra_mipi_wait(struct tegra_mipi *mipi)
 
 int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 {
-	unsigned long value;
+	unsigned long value, clk_value;
 	unsigned int i;
 	int err;
 
@@ -204,15 +240,21 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	tegra_mipi_writel(device->mipi, value, MIPI_CAL_BIAS_PAD_CFG2);
 
 	for (i = 0; i < ARRAY_SIZE(modules); i++) {
-		if (device->pads & BIT(i))
+		if (device->pads & BIT(i)) {
 			value = MIPI_CAL_CONFIG_SELECT |
 				MIPI_CAL_CONFIG_HSPDOS(0) |
 				MIPI_CAL_CONFIG_HSPUOS(4) |
 				MIPI_CAL_CONFIG_TERMOS(5);
-		else
+			clk_value = MIPI_CAL_CONFIG_SELECT |
+				    MIPI_CAL_CONFIG_HSCLKPDOSD(0) |
+				    MIPI_CAL_CONFIG_HSCLKPUOSD(4);
+		} else {
 			value = 0;
+			clk_value = 0;
+		}
 
-		tegra_mipi_writel(device->mipi, value, modules[i].reg);
+		tegra_mipi_writel(device->mipi, value, modules[i].data_reg);
+		tegra_mipi_writel(device->mipi, clk_value, modules[i].clk_reg);
 	}
 
 	value = tegra_mipi_readl(device->mipi, MIPI_CAL_CTRL);
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] host1x: mipi: Set MIPI_CAL_BIAS_PAD_CFG1 register

[Sean Paul]

During calibration, set the "internal reference level for drive
pull-down" to the value specified in the tegra TRM.

Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/gpu/host1x/mipi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 80578dc..84ed422 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -235,6 +235,8 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	value |= MIPI_CAL_BIAS_PAD_E_VCLAMP_REF;
 	tegra_mipi_writel(device->mipi, value, MIPI_CAL_BIAS_PAD_CFG0);
 
+	tegra_mipi_writel(device->mipi, 0x20000, MIPI_CAL_BIAS_PAD_CFG1);
+
 	value = tegra_mipi_readl(device->mipi, MIPI_CAL_BIAS_PAD_CFG2);
 	value &= ~MIPI_CAL_BIAS_PAD_PDVREG;
 	tegra_mipi_writel(device->mipi, value, MIPI_CAL_BIAS_PAD_CFG2);
-- 
2.0.0

Re: [PATCH 1/4] host1x: mipi: Add new parent clock for mipi calibration

[Thierry Reding]

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

On Thu, Aug 07, 2014 at 02:11:44AM -0400, Sean Paul wrote:
> This patch adds a new parent clock to enable/disable the 72MHz
> clock required for mipi calibration.

s/mipi/MIPI/ please. Also this doesn't explain why this change is
necessary. Doesn't MIPI D-PHY calibration work without this patch? It
sure does for me.

Furthermore you say 72 MHz clock, but the below uses PLL_P_OUT3 as the
parent in the example, yet PLL_P_OUT3 runs at 102 MHz on all of my
systems. What 72 MHz clock are you referring to?

Also can this parent clock ever be anything other than PLL_P_OUT3? If
not it would probably be better to set that statically in the clock
initialization tables.

> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index 9882ea1..4dd91fd 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -80,7 +80,8 @@ static const struct module {
>  struct tegra_mipi {
>  	void __iomem *regs;
>  	struct mutex lock;
> -	struct clk *clk;
> +	struct clk *clk_parent;
> +	struct clk *clk_mipi_cal;

I don't think the clk -> clk_mipi_cal rename is warranted here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/4] host1x: mipi: Preserve the contents of MIPI_CAL_CTRL

[Thierry Reding]

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

On Thu, Aug 07, 2014 at 02:11:45AM -0400, Sean Paul wrote:
> Preserve the contents of MIPI_CAL_CTRL when starting mipi
> calibration.

s/mipi/MIPI/ Also this doesn't explain why this is necessary or wanted.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] host1x: mipi: Include clock lanes in mipi calibrate

[Thierry Reding]

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

On Thu, Aug 07, 2014 at 02:11:46AM -0400, Sean Paul wrote:
> When calibrating the mipi phy, also include the clock lanes
> in the calibration.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/gpu/host1x/mipi.c | 70 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index 0af2892..80578dc 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -49,10 +49,18 @@
>  #define MIPI_CAL_CONFIG_DSIC		0x10
>  #define MIPI_CAL_CONFIG_DSID		0x11
>  
> +#define MIPI_CAL_CONFIG_DSIAB_CLK	0x19
> +#define MIPI_CAL_CONFIG_DSICD_CLK	0x1a
> +#define MIPI_CAL_CONFIG_CSIAB_CLK	0x1b
> +#define MIPI_CAL_CONFIG_CSICD_CLK	0x1c
> +#define MIPI_CAL_CONFIG_CSIE_CLK	0x1d
> +

These registers don't seem to exist on Tegra114 and earlier. It also
seems like MIPI_CAL_CONFIG_DSIC and MIPI_CAL_CONFIG_DSID no longer exist
on Tegra124 and later. And CSIC and CSID seem to be shared with DSIB
(channel A and B) now.

So I think we'll need something more elaborate than this. It should be
differentiating between SoC revisions to allow checking for valid pad
selection when calibrating.

I'll see if I can find out what's up with the change between Tegra114
and Tegra124 regarding the DSIC and DSID pads. It looks to me like they
were merged to match the DSIA and DSIB controllers, whereas before DSIA
and DSIB were used for controller DSIA and DSIC and DSID were used for
controller DSIB.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] host1x: mipi: Set MIPI_CAL_BIAS_PAD_CFG1 register

[Thierry Reding]

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

On Thu, Aug 07, 2014 at 02:11:47AM -0400, Sean Paul wrote:
> During calibration, set the "internal reference level for drive
> pull-down" to the value specified in the tegra TRM.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/gpu/host1x/mipi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index 80578dc..84ed422 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -235,6 +235,8 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
>  	value |= MIPI_CAL_BIAS_PAD_E_VCLAMP_REF;
>  	tegra_mipi_writel(device->mipi, value, MIPI_CAL_BIAS_PAD_CFG0);
>  
> +	tegra_mipi_writel(device->mipi, 0x20000, MIPI_CAL_BIAS_PAD_CFG1);

Can you add field definitions for this, please?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] host1x: mipi: Add new parent clock for mipi calibration

[Sean Paul]

On Thu, Aug 7, 2014 at 4:11 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Aug 07, 2014 at 02:11:44AM -0400, Sean Paul wrote:
>> This patch adds a new parent clock to enable/disable the 72MHz
>> clock required for mipi calibration.
>
> s/mipi/MIPI/ please. Also this doesn't explain why this change is
> necessary. Doesn't MIPI D-PHY calibration work without this patch? It
> sure does for me.

Hi Thierry,
Thanks for the prompt reviews.

It doesn't work for me on T132 without this additional clock. It seems
the source for mipi-cal has changed between T124 & T132 from PLL_OUT3
to CLK72MHZ, so that could be why it's working for you and not for me.

>
> Furthermore you say 72 MHz clock, but the below uses PLL_P_OUT3 as the
> parent in the example, yet PLL_P_OUT3 runs at 102 MHz on all of my
> systems. What 72 MHz clock are you referring to?
>

This was just a bogus assumption on my part that PLL_P_OUT3 was to be
programmed to 72MHz on pre-T132 setups.

> Also can this parent clock ever be anything other than PLL_P_OUT3? If
> not it would probably be better to set that statically in the clock
> initialization tables.

I'm not entirely certain how I'd set CLK72MHZ statically in the init
tables, could you elaborate? I can get it to work by re-parenting
mipi-cal to clk72mhz, however I'm not sure if that would break other
platforms.

My initial thought was to add a compatible = "nvidia,tegra132-mipi",
which then would require the parent to be present in the dt.

>
>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>> index 9882ea1..4dd91fd 100644
>> --- a/drivers/gpu/host1x/mipi.c
>> +++ b/drivers/gpu/host1x/mipi.c
>> @@ -80,7 +80,8 @@ static const struct module {
>>  struct tegra_mipi {
>>       void __iomem *regs;
>>       struct mutex lock;
>> -     struct clk *clk;
>> +     struct clk *clk_parent;
>> +     struct clk *clk_mipi_cal;
>
> I don't think the clk -> clk_mipi_cal rename is warranted here.

Will do.

Sean

>
> Thierry

Re: [PATCH 1/4] host1x: mipi: Add new parent clock for mipi calibration

[Thierry Reding]

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

On Thu, Aug 07, 2014 at 10:15:42AM -0400, Sean Paul wrote:
> On Thu, Aug 7, 2014 at 4:11 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Thu, Aug 07, 2014 at 02:11:44AM -0400, Sean Paul wrote:
> >> This patch adds a new parent clock to enable/disable the 72MHz
> >> clock required for mipi calibration.
> >
> > s/mipi/MIPI/ please. Also this doesn't explain why this change is
> > necessary. Doesn't MIPI D-PHY calibration work without this patch? It
> > sure does for me.
> 
> Hi Thierry,
> Thanks for the prompt reviews.
> 
> It doesn't work for me on T132 without this additional clock. It seems
> the source for mipi-cal has changed between T124 & T132 from PLL_OUT3
> to CLK72MHZ, so that could be why it's working for you and not for me.

I don't have Tegra124 hardware with DSI (I don't have Tegra132 hardware
with DSI either, for that matter) so I only tested on Tegra114. So I
guess it's possible that Tegra124 already uses clk72mhz as parent for
mipi_cal. However I can't find any authoritative source as to what
exactly is the parent on Tegra124 and later.

Peter, can you help find out what the right thing to do is here? The
clock driver currently always registers the mipi_cal clock as child of
clk_m, but that's clearly not correct. Should this be split up per SoC
so that it's a child of pll_p_out3 on Tegra114 and clk72mhz on Tegra124
and later?

> > Furthermore you say 72 MHz clock, but the below uses PLL_P_OUT3 as the
> > parent in the example, yet PLL_P_OUT3 runs at 102 MHz on all of my
> > systems. What 72 MHz clock are you referring to?
> >
> 
> This was just a bogus assumption on my part that PLL_P_OUT3 was to be
> programmed to 72MHz on pre-T132 setups.
> 
> > Also can this parent clock ever be anything other than PLL_P_OUT3? If
> > not it would probably be better to set that statically in the clock
> > initialization tables.
> 
> I'm not entirely certain how I'd set CLK72MHZ statically in the init
> tables, could you elaborate? I can get it to work by re-parenting
> mipi-cal to clk72mhz, however I'm not sure if that would break other
> platforms.

Given the above that probably won't work. The reason is that the clock
is a simple gate and uses clk_m as a parent (see the gate_clks array in
drivers/clk/tegra/clk-tegra-periph.c).

What I think should work is if we rip out the mipi_cal entry and move
registration of the clock into clk-tegra114.c and clk-tegra124.c
respectively.

> My initial thought was to add a compatible = "nvidia,tegra132-mipi",
> which then would require the parent to be present in the dt.

We need to add a new compatible anyway since there are register
differences between the two. But I'd assume that the compatible should
be "nvidia,tegra124-mipi" since it most likely remained unmodified from
Tegra124 to Tegra132.

> >> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> >> index 9882ea1..4dd91fd 100644
> >> --- a/drivers/gpu/host1x/mipi.c
> >> +++ b/drivers/gpu/host1x/mipi.c
> >> @@ -80,7 +80,8 @@ static const struct module {
> >>  struct tegra_mipi {
> >>       void __iomem *regs;
> >>       struct mutex lock;
> >> -     struct clk *clk;
> >> +     struct clk *clk_parent;
> >> +     struct clk *clk_mipi_cal;
> >
> > I don't think the clk -> clk_mipi_cal rename is warranted here.
> 
> Will do.

Given the above discussion I think this patch may simply become obsolete
if the mipi_cal clock is properly registered with pll_p_out3 or clk72mhz
as parent, since enabling the clock will then end up propagating the
enable up the whole clock tree.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] host1x: mipi: Include clock lanes in mipi calibrate

[Sean Paul]

On Thu, Aug 7, 2014 at 4:34 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Aug 07, 2014 at 02:11:46AM -0400, Sean Paul wrote:
>> When calibrating the mipi phy, also include the clock lanes
>> in the calibration.
>>
>> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  drivers/gpu/host1x/mipi.c | 70 +++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>> index 0af2892..80578dc 100644
>> --- a/drivers/gpu/host1x/mipi.c
>> +++ b/drivers/gpu/host1x/mipi.c
>> @@ -49,10 +49,18 @@
>>  #define MIPI_CAL_CONFIG_DSIC         0x10
>>  #define MIPI_CAL_CONFIG_DSID         0x11
>>
>> +#define MIPI_CAL_CONFIG_DSIAB_CLK    0x19
>> +#define MIPI_CAL_CONFIG_DSICD_CLK    0x1a
>> +#define MIPI_CAL_CONFIG_CSIAB_CLK    0x1b
>> +#define MIPI_CAL_CONFIG_CSICD_CLK    0x1c
>> +#define MIPI_CAL_CONFIG_CSIE_CLK     0x1d
>> +
>
> These registers don't seem to exist on Tegra114 and earlier. It also
> seems like MIPI_CAL_CONFIG_DSIC and MIPI_CAL_CONFIG_DSID no longer exist
> on Tegra124 and later. And CSIC and CSID seem to be shared with DSIB
> (channel A and B) now.
>
> So I think we'll need something more elaborate than this. It should be
> differentiating between SoC revisions to allow checking for valid pad
> selection when calibrating.
>

Yeah, definitely now that you point that out, we'll need something
better. I've altered the patch so the regs available depends on
compatible value. I'll wait on the following before re-posting.

> I'll see if I can find out what's up with the change between Tegra114
> and Tegra124 regarding the DSIC and DSID pads. It looks to me like they
> were merged to match the DSIA and DSIB controllers, whereas before DSIA
> and DSIB were used for controller DSIA and DSIC and DSID were used for
> controller DSIB.
>

Thanks for checking in on this. It would be useful to know which clock
lanes need to be configured for which pads. The documentation isn't
particularly good on this, so it's possible that my mapping between
data lane/clock lane in the modules array is incorrect.

Sean

> Thierry

Re: [PATCH 3/4] host1x: mipi: Include clock lanes in mipi calibrate

[Thierry Reding]

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

On Fri, Aug 22, 2014 at 10:54:50AM -0700, Sean Paul wrote:
> On Thu, Aug 7, 2014 at 10:14 AM, Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> 
> > On Thu, Aug 7, 2014 at 4:34 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8@public.gmane.orgm>
> > wrote:
> > > On Thu, Aug 07, 2014 at 02:11:46AM -0400, Sean Paul wrote:
> > >> When calibrating the mipi phy, also include the clock lanes
> > >> in the calibration.
> > >>
> > >> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > >> ---
> > >>  drivers/gpu/host1x/mipi.c | 70
> > +++++++++++++++++++++++++++++++++++++----------
> > >>  1 file changed, 56 insertions(+), 14 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> > >> index 0af2892..80578dc 100644
> > >> --- a/drivers/gpu/host1x/mipi.c
> > >> +++ b/drivers/gpu/host1x/mipi.c
> > >> @@ -49,10 +49,18 @@
> > >>  #define MIPI_CAL_CONFIG_DSIC         0x10
> > >>  #define MIPI_CAL_CONFIG_DSID         0x11
> > >>
> > >> +#define MIPI_CAL_CONFIG_DSIAB_CLK    0x19
> > >> +#define MIPI_CAL_CONFIG_DSICD_CLK    0x1a
> > >> +#define MIPI_CAL_CONFIG_CSIAB_CLK    0x1b
> > >> +#define MIPI_CAL_CONFIG_CSICD_CLK    0x1c
> > >> +#define MIPI_CAL_CONFIG_CSIE_CLK     0x1d
> > >> +
> > >
> > > These registers don't seem to exist on Tegra114 and earlier. It also
> > > seems like MIPI_CAL_CONFIG_DSIC and MIPI_CAL_CONFIG_DSID no longer exist
> > > on Tegra124 and later. And CSIC and CSID seem to be shared with DSIB
> > > (channel A and B) now.
> > >
> > > So I think we'll need something more elaborate than this. It should be
> > > differentiating between SoC revisions to allow checking for valid pad
> > > selection when calibrating.
> > >
> >
> > Yeah, definitely now that you point that out, we'll need something
> > better. I've altered the patch so the regs available depends on
> > compatible value. I'll wait on the following before re-posting.
> >
> > > I'll see if I can find out what's up with the change between Tegra114
> > > and Tegra124 regarding the DSIC and DSID pads. It looks to me like they
> > > were merged to match the DSIA and DSIB controllers, whereas before DSIA
> > > and DSIB were used for controller DSIA and DSIC and DSID were used for
> > > controller DSIB.
> > >
> >
> > Thanks for checking in on this. It would be useful to know which clock
> > lanes need to be configured for which pads. The documentation isn't
> > particularly good on this, so it's possible that my mapping between
> > data lane/clock lane in the modules array is incorrect.
> >
> >
> Hi Thierry,
> Any update on this?

I don't have any definitive answers yet, but from doing a bit of
research it seems that the D-PHY for DSI (and CSI) can be configured to
run in two modes, one where the 4 lanes are used as a single channel and
another where they can drive two channels. My understanding is that that
is what the channels refer to. That is, the D-PHY for controller DSI is
calibrated using the MIPI_CAL_CONFIG_DSIA register (0x0e/0x38) for data
lanes and MIPI_CAL_CONFIG_DSIA_A (0x19/0x64) and MIPI_CAL_CONFIG_DSIA_B
(0x1a/0x68) for clock lanes. For controller DSIB the registers are
MIPI_CAL_CONFIG_DSIB (0x0f/0x3c) and MIPI_CAL_CONFIG_DSIB_A (0x1b/0x6c)
as well as MIPI_CAL_CONFIG_DSIB_B (0x1c/0x70). The second controller
also shares the pads with CSIC and CSID.

As for the difference between Tegra114 and Tegra124 it seems that clock
lane calibration is new in Tegra124 (or perhaps it was done as part of
the data lane calibration on Tegra114), but DSIA and DSIB on Tegra114
refer to DSI channels A and B, whereas DSIC and DSID refer to channels A
and B of DSIB.

The above is still mostly guesswork, though perhaps more educated than
before. I've filed an internal bug to get confirmation from some of our
hardware engineers and clarify this in the TRM. I'll let you know when I
have more information.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]