[RFC,v2,08/14] drm: imx: Add MIPI DSI host controller driver

This patch adds i.MX MIPI DSI host controller driver support.
Currently, the driver supports the burst with sync pulses mode only.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
v1->v2:
 * Address almost all comments from Thierry Reding and Russell.
 * Update the DT documentation to remove the display-timings node in the panel node.
 * Update the DT documentation to state that the nodes which represent the possible
   DRM CRTCs the controller may connect with should be placed in the node "ports".
 * Remove the flag 'enabled' from the struct imx_mipi_dsi.
 * Move the format_to_bpp() function in v1 to the common DRM MIPI DSI driver.
 * Improve the way we wait for check status for DPHY and command packet transfer.
 * Improve the DPMS support for the encoder.
 * Split the functions of ->host_attach() and ->mode_valid() clearly as suggested by
   Thierry Reding.
 * Improve the logics in imx_mipi_dsi_dcs_long_write().
 * Enable/disable the pllref_clk and pllref_gate_clk at the component binding/unbinding
   stages to help remove the flag 'enabled'.
 * Update the module license to be "GPL".
 * Other minor changes, such as coding style issues and macro naming issues.

 .../devicetree/bindings/drm/imx/mipi_dsi.txt       |   78 ++
 drivers/gpu/drm/imx/Kconfig                        |    6 +
 drivers/gpu/drm/imx/Makefile                       |    1 +
 drivers/gpu/drm/imx/imx-mipi-dsi.c                 | 1056 ++++++++++++++++++++
 4 files changed, 1141 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
 create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c

Am Donnerstag, den 18.12.2014, 15:11 +0800 schrieb Liu Ying:
> This patch adds i.MX MIPI DSI host controller driver support.
> Currently, the driver supports the burst with sync pulses mode only.
> 
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
> v1->v2:
>  * Address almost all comments from Thierry Reding and Russell.
>  * Update the DT documentation to remove the display-timings node in the panel node.
>  * Update the DT documentation to state that the nodes which represent the possible
>    DRM CRTCs the controller may connect with should be placed in the node "ports".
>  * Remove the flag 'enabled' from the struct imx_mipi_dsi.
>  * Move the format_to_bpp() function in v1 to the common DRM MIPI DSI driver.
>  * Improve the way we wait for check status for DPHY and command packet transfer.
>  * Improve the DPMS support for the encoder.
>  * Split the functions of ->host_attach() and ->mode_valid() clearly as suggested by
>    Thierry Reding.
>  * Improve the logics in imx_mipi_dsi_dcs_long_write().
>  * Enable/disable the pllref_clk and pllref_gate_clk at the component binding/unbinding
>    stages to help remove the flag 'enabled'.
>  * Update the module license to be "GPL".
>  * Other minor changes, such as coding style issues and macro naming issues.
> 
>  .../devicetree/bindings/drm/imx/mipi_dsi.txt       |   78 ++
>  drivers/gpu/drm/imx/Kconfig                        |    6 +
>  drivers/gpu/drm/imx/Makefile                       |    1 +
>  drivers/gpu/drm/imx/imx-mipi-dsi.c                 | 1056 ++++++++++++++++++++
>  4 files changed, 1141 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
>  create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c
> 
> diff --git a/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
> new file mode 100644
> index 0000000..892ed62
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
> @@ -0,0 +1,78 @@
> +Device-Tree bindings for MIPI DSI host controller
> +
> +MIPI DSI host controller
> +========================
> +
> +The MIPI DSI host controller is a Synopsys DesignWare IP.
> +It is a digital core that implements all protocol functions defined
> +in the MIPI DSI specification, providing an interface between the
> +system and the MIPI DPHY, and allowing communication with a MIPI DSI
> +compliant display.
> +
> +Required properties:
> + - #address-cells: Should be <1>.
> + - #size-cells: Should be <0>.
> + - compatible: Should be "fsl,imx6q-mipi-dsi" for i.MX6q/sdl SoCs.

If this is a Synopsys DesignWare IP core as the HDMI TX, I think the
compatible should reflect that. How about a second compatible
"snps,dw-mipi-dsi"?

> + - reg: Physical base address of the controller and length of memory
> +         mapped region.
> + - interrupts: The controller's interrupt number to the CPU(s).
> + - gpr: Should be <&gpr>.
> +         The phandle points to the iomuxc-gpr region containing the
> +         multiplexer control register for the controller.
> + - clocks, clock-names: Phandles to the controller pllref, pllref_gate
> +           and core_cfg clocks, as described in [1] and [2].
> +
> +Required sub-nodes:
> + - ports: This node may contain up to four port nodes with endpoint
> +   definitions as defined in [3], corresponding to the four inputs to
> +   the controller multiplexer.
> + - A node to represent a DSI peripheral as described in [4].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Documentation/devicetree/bindings/clock/imx6q-clock.txt
> +[3] Documentation/devicetree/bindings/media/video-interfaces.txt
> +[4] Documentation/devicetree/bindings/mipi/dsi/mipi-dsi-bus.txt
> +
> +example:
> +	gpr: iomuxc-gpr@020e0000 {
> +		/* ... */
> +	};
> +
> +	mipi_dsi: mipi@021e0000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "fsl,imx6q-mipi-dsi";
> +		reg = <0x021e0000 0x4000>;
> +		interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
> +		gpr = <&gpr>;
> +		clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
> +			 <&clks IMX6QDL_CLK_HSI_TX>,
> +			 <&clks IMX6QDL_CLK_HSI_TX>;
> +		clock-names = "pllref", "pllref_gate", "core_cfg";

Not sure about this. Are those names from the Synopsys documentation?

According to Table 41-1 in the i.MX6Q Reference Manual, this module has
6 clock inputs:
 - ac_clk_125m (from ahb_clk_root)
 - pixel_clk (from axi_clk_root)
 - cfg_clk and pll_refclk (from video_27m)
 - ips_clk and ipg_clk_s (from ipg_clk_root)
The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk",
and "pll_refclk" are gated by a single bit called
"mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX].
If that is correct, I see no reason for the "pllref_gate" clock.
I suppose two clocks "pllref" and "cfg" should suffice.

Maybe HSI_TX should be split up into multiple shared gate clocks that
all set the mipi_core_cfg clock bits (see below).

> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> index 82fb758..03f04fb 100644
> --- a/drivers/gpu/drm/imx/Kconfig
> +++ b/drivers/gpu/drm/imx/Kconfig
> @@ -51,3 +51,9 @@ config DRM_IMX_HDMI
>  	depends on DRM_IMX
>  	help
>  	  Choose this if you want to use HDMI on i.MX6.
> +
> +config DRM_IMX_MIPI_DSI
> +	tristate "Freescale i.MX DRM MIPI DSI"
> +	depends on DRM_IMX && MFD_SYSCON
> +	help
> +	  Choose this if you want to use MIPI DSI on i.MX6.
> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
> index 582c438..4571d52 100644
> --- a/drivers/gpu/drm/imx/Makefile
> +++ b/drivers/gpu/drm/imx/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
>  imx-ipuv3-crtc-objs  := ipuv3-crtc.o ipuv3-plane.o
>  obj-$(CONFIG_DRM_IMX_IPUV3)	+= imx-ipuv3-crtc.o
>  obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
> +obj-$(CONFIG_DRM_IMX_MIPI_DSI) += imx-mipi-dsi.o
> diff --git a/drivers/gpu/drm/imx/imx-mipi-dsi.c b/drivers/gpu/drm/imx/imx-mipi-dsi.c
> new file mode 100644
> index 0000000..1cb4328
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/imx-mipi-dsi.c
[...]
> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct device_node *np = dev->of_node;
> +	struct imx_mipi_dsi *dsi;
> +	struct resource *res;
> +	u32 val;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->dsi_host.ops = &imx_mipi_dsi_host_ops;
> +	dsi->dsi_host.dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->base))
> +		return PTR_ERR(dsi->base);
> +
> +	dsi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
> +	if (IS_ERR(dsi->regmap))
> +		return PTR_ERR(dsi->regmap);
> +
> +	dsi->pllref_clk = devm_clk_get(dev, "pllref");
> +	if (IS_ERR(dsi->pllref_clk)) {
> +		ret = PTR_ERR(dsi->pllref_clk);
> +		dev_err(dev, "Unable to get pll reference clock: %d\n", ret);
> +		return ret;
> +	}
> +	clk_prepare_enable(dsi->pllref_clk);
> +
> +	dsi->pllref_gate_clk = devm_clk_get(dev, "pllref_gate");
> +	if (IS_ERR(dsi->pllref_gate_clk)) {
> +		ret = PTR_ERR(dsi->pllref_gate_clk);
> +		dev_err(dev, "Unable to get pll reference gate clock: %d\n", ret);
> +		return ret;
> +	}
> +	clk_prepare_enable(dsi->pllref_gate_clk);

As said above, I don't think this clock is needed, or is it?

If enabling pllref_clk doesn't actually enable the 27m clock input to
the mipi dsi core because it is still gated by hsi_tx, maybe the clock
tree should be fixed and hsi_tx turned into multiple
imx_clk_gate2_shared clocks.

> +
> +	dsi->cfg_clk = devm_clk_get(dev, "core_cfg");
> +	if (IS_ERR(dsi->cfg_clk)) {
> +		ret = PTR_ERR(dsi->cfg_clk);
> +		dev_err(dev, "Unable to get configuration clock: %d\n", ret);

And leave pllref enabled?

> +		return ret;
> +	}
> +
> +	clk_prepare_enable(dsi->cfg_clk);
> +	val = dsi_read(dsi, DSI_VERSION);
> +	clk_disable_unprepare(dsi->cfg_clk);
> +
> +	dev_info(dev, "version number is 0x%08x\n", val);
> +
> +	ret = imx_mipi_dsi_register(drm, dsi);
> +	if (ret)

Same here.

> +		return ret;
> +
> +	dev_set_drvdata(dev, dsi);
> +
> +	return mipi_dsi_host_register(&dsi->dsi_host);
> +}
[...]

regards
Philipp

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

On 12/18/2014 07:39 PM, Philipp Zabel wrote:
> Am Donnerstag, den 18.12.2014, 15:11 +0800 schrieb Liu Ying:
>> This patch adds i.MX MIPI DSI host controller driver support.
>> Currently, the driver supports the burst with sync pulses mode only.
>>
>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>> ---
>> v1->v2:
>>   * Address almost all comments from Thierry Reding and Russell.
>>   * Update the DT documentation to remove the display-timings node in the panel node.
>>   * Update the DT documentation to state that the nodes which represent the possible
>>     DRM CRTCs the controller may connect with should be placed in the node "ports".
>>   * Remove the flag 'enabled' from the struct imx_mipi_dsi.
>>   * Move the format_to_bpp() function in v1 to the common DRM MIPI DSI driver.
>>   * Improve the way we wait for check status for DPHY and command packet transfer.
>>   * Improve the DPMS support for the encoder.
>>   * Split the functions of ->host_attach() and ->mode_valid() clearly as suggested by
>>     Thierry Reding.
>>   * Improve the logics in imx_mipi_dsi_dcs_long_write().
>>   * Enable/disable the pllref_clk and pllref_gate_clk at the component binding/unbinding
>>     stages to help remove the flag 'enabled'.
>>   * Update the module license to be "GPL".
>>   * Other minor changes, such as coding style issues and macro naming issues.
>>
>>   .../devicetree/bindings/drm/imx/mipi_dsi.txt       |   78 ++
>>   drivers/gpu/drm/imx/Kconfig                        |    6 +
>>   drivers/gpu/drm/imx/Makefile                       |    1 +
>>   drivers/gpu/drm/imx/imx-mipi-dsi.c                 | 1056 ++++++++++++++++++++
>>   4 files changed, 1141 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
>>   create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c
>>
>> diff --git a/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
>> new file mode 100644
>> index 0000000..892ed62
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
>> @@ -0,0 +1,78 @@
>> +Device-Tree bindings for MIPI DSI host controller
>> +
>> +MIPI DSI host controller
>> +========================
>> +
>> +The MIPI DSI host controller is a Synopsys DesignWare IP.
>> +It is a digital core that implements all protocol functions defined
>> +in the MIPI DSI specification, providing an interface between the
>> +system and the MIPI DPHY, and allowing communication with a MIPI DSI
>> +compliant display.
>> +
>> +Required properties:
>> + - #address-cells: Should be <1>.
>> + - #size-cells: Should be <0>.
>> + - compatible: Should be "fsl,imx6q-mipi-dsi" for i.MX6q/sdl SoCs.
>
> If this is a Synopsys DesignWare IP core as the HDMI TX, I think the
> compatible should reflect that. How about a second compatible
> "snps,dw-mipi-dsi"?

Ok, I'll add this second compatible string.

>
>> + - reg: Physical base address of the controller and length of memory
>> +         mapped region.
>> + - interrupts: The controller's interrupt number to the CPU(s).
>> + - gpr: Should be <&gpr>.
>> +         The phandle points to the iomuxc-gpr region containing the
>> +         multiplexer control register for the controller.
>> + - clocks, clock-names: Phandles to the controller pllref, pllref_gate
>> +           and core_cfg clocks, as described in [1] and [2].
>> +
>> +Required sub-nodes:
>> + - ports: This node may contain up to four port nodes with endpoint
>> +   definitions as defined in [3], corresponding to the four inputs to
>> +   the controller multiplexer.
>> + - A node to represent a DSI peripheral as described in [4].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +[2] Documentation/devicetree/bindings/clock/imx6q-clock.txt
>> +[3] Documentation/devicetree/bindings/media/video-interfaces.txt
>> +[4] Documentation/devicetree/bindings/mipi/dsi/mipi-dsi-bus.txt
>> +
>> +example:
>> +	gpr: iomuxc-gpr@020e0000 {
>> +		/* ... */
>> +	};
>> +
>> +	mipi_dsi: mipi@021e0000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		compatible = "fsl,imx6q-mipi-dsi";
>> +		reg = <0x021e0000 0x4000>;
>> +		interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
>> +		gpr = <&gpr>;
>> +		clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
>> +			 <&clks IMX6QDL_CLK_HSI_TX>,
>> +			 <&clks IMX6QDL_CLK_HSI_TX>;
>> +		clock-names = "pllref", "pllref_gate", "core_cfg";
>
> Not sure about this. Are those names from the Synopsys documentation?

No, I don't think it's from there.

>
> According to Table 41-1 in the i.MX6Q Reference Manual, this module has
> 6 clock inputs:
>   - ac_clk_125m (from ahb_clk_root)
>   - pixel_clk (from axi_clk_root)
>   - cfg_clk and pll_refclk (from video_27m)
>   - ips_clk and ipg_clk_s (from ipg_clk_root)
> The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk",
> and "pll_refclk" are gated by a single bit called
> "mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX].
> If that is correct, I see no reason for the "pllref_gate" clock.
> I suppose two clocks "pllref" and "cfg" should suffice.

Using the two clocks makes the code look like this, perhaps:

       clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
                <&clks IMX6QDL_CLK_HSI_TX>;
       clock-names = "pllref", "core_cfg";

Then, it seems that I have no way to disable the pllref clock if
using the clock tree after applying this patch set?


Or, perhaps, this one?

       clocks = <&clks IMX6QDL_CLK_HSI_TX>,
                <&clks IMX6QDL_CLK_HSI_TX>;
       clock-names = "pllref", "core_cfg";

This only uses the gate clock hsi_tx.  The current clock tree states
that it comes from:

      pll3_120m ->
                  | -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
pll2_pfd2_396m ->

So, I can not get the correct pllref clock frequency with this tree.
The pllref clock should be derived from the video_27m clock.


The way I decided to use the three clocks is:
1) PLL related
* pllref clock only cares about the pll reference rate(the frequency).
   And, the frequency does matter as it has an impact on the lane clock
   frequency.
* pllref_gate is a gate clock and it only cares about the gate.

2) register configuration related
* core_cfg is a gate clock and it only cares about the gate.
Usually, the register configuration clock frequency is not so important
and the gate is what we really need.

I am currently not strong on the way I used.  I am open to any better
solution.

>
> Maybe HSI_TX should be split up into multiple shared gate clocks that
> all set the mipi_core_cfg clock bits (see below).

Yes, maybe.
If that's the case, do we need to add two gate clocks in the DT node to
represent cfg_gate and pllref_gate respectively?

>
>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
>> index 82fb758..03f04fb 100644
>> --- a/drivers/gpu/drm/imx/Kconfig
>> +++ b/drivers/gpu/drm/imx/Kconfig
>> @@ -51,3 +51,9 @@ config DRM_IMX_HDMI
>>   	depends on DRM_IMX
>>   	help
>>   	  Choose this if you want to use HDMI on i.MX6.
>> +
>> +config DRM_IMX_MIPI_DSI
>> +	tristate "Freescale i.MX DRM MIPI DSI"
>> +	depends on DRM_IMX && MFD_SYSCON
>> +	help
>> +	  Choose this if you want to use MIPI DSI on i.MX6.
>> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
>> index 582c438..4571d52 100644
>> --- a/drivers/gpu/drm/imx/Makefile
>> +++ b/drivers/gpu/drm/imx/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
>>   imx-ipuv3-crtc-objs  := ipuv3-crtc.o ipuv3-plane.o
>>   obj-$(CONFIG_DRM_IMX_IPUV3)	+= imx-ipuv3-crtc.o
>>   obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
>> +obj-$(CONFIG_DRM_IMX_MIPI_DSI) += imx-mipi-dsi.o
>> diff --git a/drivers/gpu/drm/imx/imx-mipi-dsi.c b/drivers/gpu/drm/imx/imx-mipi-dsi.c
>> new file mode 100644
>> index 0000000..1cb4328
>> --- /dev/null
>> +++ b/drivers/gpu/drm/imx/imx-mipi-dsi.c
> [...]
>> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct drm_device *drm = data;
>> +	struct device_node *np = dev->of_node;
>> +	struct imx_mipi_dsi *dsi;
>> +	struct resource *res;
>> +	u32 val;
>> +	int ret;
>> +
>> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
>> +	if (!dsi)
>> +		return -ENOMEM;
>> +
>> +	dsi->dev = dev;
>> +	dsi->dsi_host.ops = &imx_mipi_dsi_host_ops;
>> +	dsi->dsi_host.dev = dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	dsi->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(dsi->base))
>> +		return PTR_ERR(dsi->base);
>> +
>> +	dsi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
>> +	if (IS_ERR(dsi->regmap))
>> +		return PTR_ERR(dsi->regmap);
>> +
>> +	dsi->pllref_clk = devm_clk_get(dev, "pllref");
>> +	if (IS_ERR(dsi->pllref_clk)) {
>> +		ret = PTR_ERR(dsi->pllref_clk);
>> +		dev_err(dev, "Unable to get pll reference clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +	clk_prepare_enable(dsi->pllref_clk);
>> +
>> +	dsi->pllref_gate_clk = devm_clk_get(dev, "pllref_gate");
>> +	if (IS_ERR(dsi->pllref_gate_clk)) {
>> +		ret = PTR_ERR(dsi->pllref_gate_clk);
>> +		dev_err(dev, "Unable to get pll reference gate clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +	clk_prepare_enable(dsi->pllref_gate_clk);
>
> As said above, I don't think this clock is needed, or is it?

Perhaps, we need it.

>
> If enabling pllref_clk doesn't actually enable the 27m clock input to
> the mipi dsi core because it is still gated by hsi_tx, maybe the clock
> tree should be fixed and hsi_tx turned into multiple
> imx_clk_gate2_shared clocks.

According to the CCM chapter, the video_27m clock is gated by the hsi_tx
clock.  You mentioned this above, as well.

>
>> +
>> +	dsi->cfg_clk = devm_clk_get(dev, "core_cfg");
>> +	if (IS_ERR(dsi->cfg_clk)) {
>> +		ret = PTR_ERR(dsi->cfg_clk);
>> +		dev_err(dev, "Unable to get configuration clock: %d\n", ret);
>
> And leave pllref enabled?

As I mentioned in the v1-> v2 change log, I enable/disable the 
pllref_clk and pllref_gate_clk at the component binding/unbinding stages 
to help remove the flag 'enabled' introduced in v1.

I referred to the i.MX HDMI driver which enables/disables the isfr clock 
and the iahb clock at the component binding/unbinding stages.

I may try to handle the clock enablement/disablement more decently and
avoid using the flag 'enable'.

>
>> +		return ret;
>> +	}
>> +
>> +	clk_prepare_enable(dsi->cfg_clk);
>> +	val = dsi_read(dsi, DSI_VERSION);
>> +	clk_disable_unprepare(dsi->cfg_clk);
>> +
>> +	dev_info(dev, "version number is 0x%08x\n", val);
>> +
>> +	ret = imx_mipi_dsi_register(drm, dsi);
>> +	if (ret)
>
> Same here.

You meant that the pllref_gate clock is left enabled above, right?

Regards,
Liu Ying

>
>> +		return ret;
>> +
>> +	dev_set_drvdata(dev, dsi);
>> +
>> +	return mipi_dsi_host_register(&dsi->dsi_host);
>> +}
> [...]
>
> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Liu,

Am Freitag, den 19.12.2014, 13:53 +0800 schrieb Liu Ying:
[...]
> >> +	mipi_dsi: mipi@021e0000 {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +		compatible = "fsl,imx6q-mipi-dsi";
> >> +		reg = <0x021e0000 0x4000>;
> >> +		interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
> >> +		gpr = <&gpr>;
> >> +		clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
> >> +			 <&clks IMX6QDL_CLK_HSI_TX>,
> >> +			 <&clks IMX6QDL_CLK_HSI_TX>;
> >> +		clock-names = "pllref", "pllref_gate", "core_cfg";
> >
> > Not sure about this. Are those names from the Synopsys documentation?
> 
> No, I don't think it's from there.

Do you have access to it? I'd like to see the proper names used if
possible, considering this IP core will be used on other SoCs, too.

> > According to Table 41-1 in the i.MX6Q Reference Manual, this module has
> > 6 clock inputs:
> >   - ac_clk_125m (from ahb_clk_root)
> >   - pixel_clk (from axi_clk_root)
> >   - cfg_clk and pll_refclk (from video_27m)
> >   - ips_clk and ipg_clk_s (from ipg_clk_root)
> > The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk",
> > and "pll_refclk" are gated by a single bit called
> > "mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX].
> > If that is correct, I see no reason for the "pllref_gate" clock.
> > I suppose two clocks "pllref" and "cfg" should suffice.
> 
> Using the two clocks makes the code look like this, perhaps:
> 
>        clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
>                 <&clks IMX6QDL_CLK_HSI_TX>;
>        clock-names = "pllref", "core_cfg";
> 
> Then, it seems that I have no way to disable the pllref clock if
> using the clock tree after applying this patch set?

Correct. In my opinion we should put a new gate clock in the clock path
between video_27m and the pllref input triggers the same bit as hsi_tx,
see below.

> Or, perhaps, this one?
> 
>        clocks = <&clks IMX6QDL_CLK_HSI_TX>,
>                 <&clks IMX6QDL_CLK_HSI_TX>;
>        clock-names = "pllref", "core_cfg";
> 
> This only uses the gate clock hsi_tx.  The current clock tree states
> that it comes from:
> 
>       pll3_120m ->
>                   | -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
> pll2_pfd2_396m ->
> 
> So, I can not get the correct pllref clock frequency with this tree.
> The pllref clock should be derived from the video_27m clock.

According to the i.MX6 reference manual, the cfg clock also is derived
from video_27m, so both have the wrong rate if connected to hsi_tx (yes,
for cfg we don't care about the rate).

Currently we have this:

pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
pll3_pfd1_540m -> video_27m -> hdmi_isfr

- hsi_tx_podf represents the hsi_tx_clk_root at its output.
- hsi_tx represents the gate between hsi_tx_clk_root and the tx_ref_clk
  input to the MIPI_HSI module, which is controlled by the
  mipi_core_cfg_clk_enable bit.
- video_27m represents the video_27m_clk_root at its output.

I'd say we should turn hsi_tx into a shared gate clock and add a new
shared gate clock using the same gate bit. Maybe name it mipi_core_cfg,
after the gating bit in the CCM.

pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
pll3_pfd1_540m -> video_27m -> mipi_core_cfg
pll3_pfd1_540m -> video_27m -> hdmi_isfr

- mipi_core_cfg represents the gate between video_27_clk_root and the
  cfg_clk and pllref_clk inputs to the MIPI_DSI module, which is also
  controlled by the mipi_core_cfg_clk_enable bit.

> The way I decided to use the three clocks is:
> 1) PLL related
> * pllref clock only cares about the pll reference rate(the frequency).
>    And, the frequency does matter as it has an impact on the lane clock
>    frequency.
> * pllref_gate is a gate clock and it only cares about the gate.
>
> 2) register configuration related
> * core_cfg is a gate clock and it only cares about the gate.
> Usually, the register configuration clock frequency is not so important
> and the gate is what we really need.
> 
> I am currently not strong on the way I used.  I am open to any better
> solution.

Since cfg is a real clock input to the MIPI DSI IP, that's ok. But the
two pllref entries in reality are one and the same clock input.

> > Maybe HSI_TX should be split up into multiple shared gate clocks that
> > all set the mipi_core_cfg clock bits (see below).
> 
> Yes, maybe.
> If that's the case, do we need to add two gate clocks in the DT node to
> represent cfg_gate and pllref_gate respectively?

I'd say yes. While on i.MX6 it could be represented by a single clock
because both have the same rate and use the same gate bit, that doesn't
have to be the case on other SoCs. With my suggestion above, that would
be:

	clocks = <&clks IMX6QDL_CLK_MIPI_CORE_CFG>,
		 <&clks IMX6QDL_CLK_MIPI_CORE_CFG>;
	clock-names = "pllref", "cfg";

> >> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
[...]
> >> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data)
> >> +{
[...]
> >> +	dsi->pllref_clk = devm_clk_get(dev, "pllref");
> >> +	if (IS_ERR(dsi->pllref_clk)) {
> >> +		ret = PTR_ERR(dsi->pllref_clk);
> >> +		dev_err(dev, "Unable to get pll reference clock: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +	clk_prepare_enable(dsi->pllref_clk);

What I mean below is this: Here you enable pllref ...

[...]
> >> +	dsi->cfg_clk = devm_clk_get(dev, "core_cfg");
> >> +	if (IS_ERR(dsi->cfg_clk)) {
> >> +		ret = PTR_ERR(dsi->cfg_clk);
> >> +		dev_err(dev, "Unable to get configuration clock: %d\n", ret);
> >
> > And leave pllref enabled?
> 
> As I mentioned in the v1-> v2 change log, I enable/disable the 
> pllref_clk and pllref_gate_clk at the component binding/unbinding stages 
> to help remove the flag 'enabled' introduced in v1.
> 
> I referred to the i.MX HDMI driver which enables/disables the isfr clock 
> and the iahb clock at the component binding/unbinding stages.
> 
> I may try to handle the clock enablement/disablement more decently and
> avoid using the flag 'enable'.
> 
> >
> >> +		return ret;

... and here you return with an error without disabling pllref again. If
the bind fails, unbind won't be called, and the clock stays enabled. For
reference, see how imx-hdmi unprepare_disables its iahb/isfr clocks in
the bind function's error path.

> >> +	}
> >> +
> >> +	clk_prepare_enable(dsi->cfg_clk);
> >> +	val = dsi_read(dsi, DSI_VERSION);
> >> +	clk_disable_unprepare(dsi->cfg_clk);
> >> +
> >> +	dev_info(dev, "version number is 0x%08x\n", val);
> >> +
> >> +	ret = imx_mipi_dsi_register(drm, dsi);
> >> +	if (ret)
> >
> > Same here.
> 
> You meant that the pllref_gate clock is left enabled above, right?

Yes.

> Regards,
> Liu Ying
> 
> >
> >> +		return ret;

This return with an error leaves the pllref enabled.

regards
Philipp

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

Hi Philipp,

On 12/19/2014 06:17 PM, Philipp Zabel wrote:
> Hi Liu,
>
> Am Freitag, den 19.12.2014, 13:53 +0800 schrieb Liu Ying:
> [...]
>>>> +	mipi_dsi: mipi@021e0000 {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +		compatible = "fsl,imx6q-mipi-dsi";
>>>> +		reg = <0x021e0000 0x4000>;
>>>> +		interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
>>>> +		gpr = <&gpr>;
>>>> +		clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
>>>> +			 <&clks IMX6QDL_CLK_HSI_TX>,
>>>> +			 <&clks IMX6QDL_CLK_HSI_TX>;
>>>> +		clock-names = "pllref", "pllref_gate", "core_cfg";
>>>
>>> Not sure about this. Are those names from the Synopsys documentation?
>>
>> No, I don't think it's from there.
>
> Do you have access to it? I'd like to see the proper names used if
> possible, considering this IP core will be used on other SoCs, too.

I'm using the Synopsys documentation for Freescale copy.
I'm not sure if it may be provided in the open mailing lists.

You probably may try [1] to require one copy from Synopsys.

[1] https://www.synopsys.com/dw/ipdir.php?ds=mipi_dsi

>
>>> According to Table 41-1 in the i.MX6Q Reference Manual, this module has
>>> 6 clock inputs:
>>>    - ac_clk_125m (from ahb_clk_root)
>>>    - pixel_clk (from axi_clk_root)
>>>    - cfg_clk and pll_refclk (from video_27m)
>>>    - ips_clk and ipg_clk_s (from ipg_clk_root)
>>> The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk",
>>> and "pll_refclk" are gated by a single bit called
>>> "mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX].
>>> If that is correct, I see no reason for the "pllref_gate" clock.
>>> I suppose two clocks "pllref" and "cfg" should suffice.
>>
>> Using the two clocks makes the code look like this, perhaps:
>>
>>         clocks = <&clks IMX6QDL_CLK_VIDEO_27M>,
>>                  <&clks IMX6QDL_CLK_HSI_TX>;
>>         clock-names = "pllref", "core_cfg";
>>
>> Then, it seems that I have no way to disable the pllref clock if
>> using the clock tree after applying this patch set?
>
> Correct. In my opinion we should put a new gate clock in the clock path
> between video_27m and the pllref input triggers the same bit as hsi_tx,
> see below.
>
>> Or, perhaps, this one?
>>
>>         clocks = <&clks IMX6QDL_CLK_HSI_TX>,
>>                  <&clks IMX6QDL_CLK_HSI_TX>;
>>         clock-names = "pllref", "core_cfg";
>>
>> This only uses the gate clock hsi_tx.  The current clock tree states
>> that it comes from:
>>
>>        pll3_120m ->
>>                    | -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
>> pll2_pfd2_396m ->
>>
>> So, I can not get the correct pllref clock frequency with this tree.
>> The pllref clock should be derived from the video_27m clock.
>
> According to the i.MX6 reference manual, the cfg clock also is derived
> from video_27m, so both have the wrong rate if connected to hsi_tx (yes,
> for cfg we don't care about the rate).
>
> Currently we have this:
>
> pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
> pll3_pfd1_540m -> video_27m -> hdmi_isfr
>
> - hsi_tx_podf represents the hsi_tx_clk_root at its output.
> - hsi_tx represents the gate between hsi_tx_clk_root and the tx_ref_clk
>    input to the MIPI_HSI module, which is controlled by the
>    mipi_core_cfg_clk_enable bit.
> - video_27m represents the video_27m_clk_root at its output.
>
> I'd say we should turn hsi_tx into a shared gate clock and add a new
> shared gate clock using the same gate bit. Maybe name it mipi_core_cfg,
> after the gating bit in the CCM.
>
> pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx
> pll3_pfd1_540m -> video_27m -> mipi_core_cfg
> pll3_pfd1_540m -> video_27m -> hdmi_isfr
>
> - mipi_core_cfg represents the gate between video_27_clk_root and the
>    cfg_clk and pllref_clk inputs to the MIPI_DSI module, which is also
>    controlled by the mipi_core_cfg_clk_enable bit.
>
>> The way I decided to use the three clocks is:
>> 1) PLL related
>> * pllref clock only cares about the pll reference rate(the frequency).
>>     And, the frequency does matter as it has an impact on the lane clock
>>     frequency.
>> * pllref_gate is a gate clock and it only cares about the gate.
>>
>> 2) register configuration related
>> * core_cfg is a gate clock and it only cares about the gate.
>> Usually, the register configuration clock frequency is not so important
>> and the gate is what we really need.
>>
>> I am currently not strong on the way I used.  I am open to any better
>> solution.
>
> Since cfg is a real clock input to the MIPI DSI IP, that's ok. But the
> two pllref entries in reality are one and the same clock input.
>
>>> Maybe HSI_TX should be split up into multiple shared gate clocks that
>>> all set the mipi_core_cfg clock bits (see below).
>>
>> Yes, maybe.
>> If that's the case, do we need to add two gate clocks in the DT node to
>> represent cfg_gate and pllref_gate respectively?
>
> I'd say yes. While on i.MX6 it could be represented by a single clock
> because both have the same rate and use the same gate bit, that doesn't
> have to be the case on other SoCs. With my suggestion above, that would
> be:
>
> 	clocks = <&clks IMX6QDL_CLK_MIPI_CORE_CFG>,
> 		 <&clks IMX6QDL_CLK_MIPI_CORE_CFG>;
> 	clock-names = "pllref", "cfg";

Your suggestion looks better. I'll implement it in the next version
and give you the "Suggested-by" credit. Thanks.

>
>>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> [...]
>>>> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data)
>>>> +{
> [...]
>>>> +	dsi->pllref_clk = devm_clk_get(dev, "pllref");
>>>> +	if (IS_ERR(dsi->pllref_clk)) {
>>>> +		ret = PTR_ERR(dsi->pllref_clk);
>>>> +		dev_err(dev, "Unable to get pll reference clock: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +	clk_prepare_enable(dsi->pllref_clk);
>
> What I mean below is this: Here you enable pllref ...
>
> [...]
>>>> +	dsi->cfg_clk = devm_clk_get(dev, "core_cfg");
>>>> +	if (IS_ERR(dsi->cfg_clk)) {
>>>> +		ret = PTR_ERR(dsi->cfg_clk);
>>>> +		dev_err(dev, "Unable to get configuration clock: %d\n", ret);
>>>
>>> And leave pllref enabled?
>>
>> As I mentioned in the v1-> v2 change log, I enable/disable the
>> pllref_clk and pllref_gate_clk at the component binding/unbinding stages
>> to help remove the flag 'enabled' introduced in v1.
>>
>> I referred to the i.MX HDMI driver which enables/disables the isfr clock
>> and the iahb clock at the component binding/unbinding stages.
>>
>> I may try to handle the clock enablement/disablement more decently and
>> avoid using the flag 'enable'.
>>
>>>
>>>> +		return ret;
>
> ... and here you return with an error without disabling pllref again. If
> the bind fails, unbind won't be called, and the clock stays enabled. For
> reference, see how imx-hdmi unprepare_disables its iahb/isfr clocks in
> the bind function's error path.

I'll improve the logic for the bail-out path.

Regards,
Liu Ying

>
>>>> +	}
>>>> +
>>>> +	clk_prepare_enable(dsi->cfg_clk);
>>>> +	val = dsi_read(dsi, DSI_VERSION);
>>>> +	clk_disable_unprepare(dsi->cfg_clk);
>>>> +
>>>> +	dev_info(dev, "version number is 0x%08x\n", val);
>>>> +
>>>> +	ret = imx_mipi_dsi_register(drm, dsi);
>>>> +	if (ret)
>>>
>>> Same here.
>>
>> You meant that the pllref_gate clock is left enabled above, right?
>
> Yes.
>
>> Regards,
>> Liu Ying
>>
>>>
>>>> +		return ret;
>
> This return with an error leaves the pllref enabled.
>
> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html