[PATCH 0/4] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[PATCH 0/4] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Liu Ying]

Hi,

This series adds i.MX8qxp LVDS PHY mode support for the Mixel PHY in the
Freescale i.MX8qxp SoC.

The Mixel PHY is MIPI DPHY + LVDS PHY combo, which can works in either
MIPI DPHY mode or LVDS PHY mode.  The PHY mode is controlled by i.MX8qxp
SCU firmware.  The PHY driver would call a SCU function to configure the
mode.

The PHY driver is already supporting the Mixel MIPI DPHY in i.MX8mq SoC,
where it appears to be a single MIPI DPHY.


Patch 1/4 sets PHY mode in the Northwest Logic MIPI DSI host controller
bridge driver, since i.MX8qxp SoC embeds this controller IP to support
MIPI DSI displays together with the Mixel PHY.

Patch 2/4 allows LVDS PHYs to be configured through the generic PHY functions
and through a custom structure added to the generic PHY configuration union.

Patch 3/4 adds dt binding support for the Mixel combo PHY in i.MX8qxp SoC.

Patch 4/4 adds the i.MX8qxp LVDS PHY mode support in the Mixel PHY driver.


Welcome comments, thanks.


Liu Ying (4):
  drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()
  phy: Add LVDS configuration options
  dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for
    i.MX8qxp
  phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode
    support

 .../devicetree/bindings/phy/mixel,mipi-dsi-phy.txt |   8 +-
 drivers/gpu/drm/bridge/nwl-dsi.c                   |   6 +
 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c     | 266 ++++++++++++++++++++-
 include/linux/phy/phy-lvds.h                       |  48 ++++
 include/linux/phy/phy.h                            |   4 +
 5 files changed, 320 insertions(+), 12 deletions(-)
 create mode 100644 include/linux/phy/phy-lvds.h

-- 
2.7.4

[PATCH 1/4] drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()

[Liu Ying]

The Northwest Logic MIPI DSI host controller embedded in i.MX8qxp
works with a Mixel MIPI DPHY + LVDS PHY combo to support either
a MIPI DSI display or a LVDS display.  So, this patch calls
phy_set_mode() from nwl_dsi_enable() to set PHY mode to MIPI DPHY
explicitly.

Cc: Guido Günther <agx@sigxcpu.org>
Cc: Robert Chiras <robert.chiras@nxp.com>
Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/nwl-dsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index 66b6740..be6bfc5 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -678,6 +678,12 @@ static int nwl_dsi_enable(struct nwl_dsi *dsi)
 		return ret;
 	}
 
+	ret = phy_set_mode(dsi->phy, PHY_MODE_MIPI_DPHY);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "Failed to set DSI phy mode: %d\n", ret);
+		goto uninit_phy;
+	}
+
 	ret = phy_configure(dsi->phy, phy_cfg);
 	if (ret < 0) {
 		DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
-- 
2.7.4

Re: [PATCH 1/4] drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()

[Guido Günther]

Hi,
On Fri, Dec 04, 2020 at 03:33:41PM +0800, Liu Ying wrote:
> The Northwest Logic MIPI DSI host controller embedded in i.MX8qxp
> works with a Mixel MIPI DPHY + LVDS PHY combo to support either
> a MIPI DSI display or a LVDS display.  So, this patch calls
> phy_set_mode() from nwl_dsi_enable() to set PHY mode to MIPI DPHY
> explicitly.
> 
> Cc: Guido Günther <agx@sigxcpu.org>
> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/nwl-dsi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index 66b6740..be6bfc5 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -678,6 +678,12 @@ static int nwl_dsi_enable(struct nwl_dsi *dsi)
>  		return ret;
>  	}
>  
> +	ret = phy_set_mode(dsi->phy, PHY_MODE_MIPI_DPHY);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to set DSI phy mode: %d\n", ret);
> +		goto uninit_phy;
> +	}
> +
>  	ret = phy_configure(dsi->phy, phy_cfg);
>  	if (ret < 0) {
>  		DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);

Reviewed-by: Guido Günther <agx@sigxcpu.org> 
 -- Guido

> -- 
> 2.7.4
> 

Re: [PATCH 1/4] drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()

[Guido Günther]

Hi,
On Tue, Dec 08, 2020 at 10:04:57AM +0100, Guido Günther wrote:
> Hi,
> On Fri, Dec 04, 2020 at 03:33:41PM +0800, Liu Ying wrote:
> > The Northwest Logic MIPI DSI host controller embedded in i.MX8qxp
> > works with a Mixel MIPI DPHY + LVDS PHY combo to support either
> > a MIPI DSI display or a LVDS display.  So, this patch calls
> > phy_set_mode() from nwl_dsi_enable() to set PHY mode to MIPI DPHY
> > explicitly.

Should i pull this patch in via drm-misc-next or is the whole series
supposed to go via the phy tree?
Cheers,
 -- Guido


> > 
> > Cc: Guido Günther <agx@sigxcpu.org>
> > Cc: Robert Chiras <robert.chiras@nxp.com>
> > Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/nwl-dsi.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> > index 66b6740..be6bfc5 100644
> > --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> > +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> > @@ -678,6 +678,12 @@ static int nwl_dsi_enable(struct nwl_dsi *dsi)
> >  		return ret;
> >  	}
> >  
> > +	ret = phy_set_mode(dsi->phy, PHY_MODE_MIPI_DPHY);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(dev, "Failed to set DSI phy mode: %d\n", ret);
> > +		goto uninit_phy;
> > +	}
> > +
> >  	ret = phy_configure(dsi->phy, phy_cfg);
> >  	if (ret < 0) {
> >  		DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
> 
> Reviewed-by: Guido Günther <agx@sigxcpu.org> 
>  -- Guido
> 
> > -- 
> > 2.7.4
> > 
> 
> 

[PATCH 2/4] phy: Add LVDS configuration options

[Liu Ying]

This patch allows LVDS PHYs to be configured through
the generic functions and through a custom structure
added to the generic union.

The parameters added here are based on common LVDS PHY
implementation practices.  The set of parameters
should cover all potential users.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 include/linux/phy/phy-lvds.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h      |  4 ++++
 2 files changed, 52 insertions(+)
 create mode 100644 include/linux/phy/phy-lvds.h

diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h
new file mode 100644
index 00000000..1b5b9d6
--- /dev/null
+++ b/include/linux/phy/phy-lvds.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2020 NXP
+ */
+
+#ifndef __PHY_LVDS_H_
+#define __PHY_LVDS_H_
+
+/**
+ * struct phy_configure_opts_lvds - LVDS configuration set
+ *
+ * This structure is used to represent the configuration state of a
+ * LVDS phy.
+ */
+struct phy_configure_opts_lvds {
+	/**
+	 * @bits_per_lane_and_dclk_cycle:
+	 *
+	 * Number of bits per data lane and differential clock cycle.
+	 */
+	unsigned int bits_per_lane_and_dclk_cycle;
+
+	/**
+	 * @differential_clk_rate:
+	 *
+	 * Clock rate, in Hertz, of the LVDS differential clock.
+	 */
+	unsigned long differential_clk_rate;
+
+	/**
+	 * @lanes:
+	 *
+	 * Number of active, consecutive, data lanes, starting from
+	 * lane 0, used for the transmissions.
+	 */
+	unsigned int lanes;
+
+	/**
+	 * @is_slave:
+	 *
+	 * Boolean, true if the phy is a slave which works together
+	 * with a master phy to support dual link transmission,
+	 * otherwise a regular phy or a master phy.
+	 */
+	bool is_slave;
+};
+
+#endif /* __PHY_LVDS_H_ */
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index e435bdb..d450b44 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -17,6 +17,7 @@
 #include <linux/regulator/consumer.h>
 
 #include <linux/phy/phy-dp.h>
+#include <linux/phy/phy-lvds.h>
 #include <linux/phy/phy-mipi-dphy.h>
 
 struct phy;
@@ -51,10 +52,13 @@ enum phy_mode {
  *		the MIPI_DPHY phy mode.
  * @dp:		Configuration set applicable for phys supporting
  *		the DisplayPort protocol.
+ * @lvds:	Configuration set applicable for phys supporting
+ *		the LVDS phy mode.
  */
 union phy_configure_opts {
 	struct phy_configure_opts_mipi_dphy	mipi_dphy;
 	struct phy_configure_opts_dp		dp;
+	struct phy_configure_opts_lvds		lvds;
 };
 
 /**
-- 
2.7.4

Re: [PATCH 2/4] phy: Add LVDS configuration options

[Laurent Pinchart]

Hi Liu,

Thank you for the patch.

On Fri, Dec 04, 2020 at 03:33:42PM +0800, Liu Ying wrote:
> This patch allows LVDS PHYs to be configured through
> the generic functions and through a custom structure
> added to the generic union.
> 
> The parameters added here are based on common LVDS PHY
> implementation practices.  The set of parameters
> should cover all potential users.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  include/linux/phy/phy-lvds.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h      |  4 ++++
>  2 files changed, 52 insertions(+)
>  create mode 100644 include/linux/phy/phy-lvds.h
> 
> diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h
> new file mode 100644
> index 00000000..1b5b9d6
> --- /dev/null
> +++ b/include/linux/phy/phy-lvds.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2020 NXP
> + */
> +
> +#ifndef __PHY_LVDS_H_
> +#define __PHY_LVDS_H_
> +
> +/**
> + * struct phy_configure_opts_lvds - LVDS configuration set
> + *
> + * This structure is used to represent the configuration state of a
> + * LVDS phy.
> + */
> +struct phy_configure_opts_lvds {
> +	/**
> +	 * @bits_per_lane_and_dclk_cycle:
> +	 *
> +	 * Number of bits per data lane and differential clock cycle.
> +	 */
> +	unsigned int bits_per_lane_and_dclk_cycle;

I see in patch 4/4 that you only support 7, can the value be any
different ?

> +
> +	/**
> +	 * @differential_clk_rate:
> +	 *
> +	 * Clock rate, in Hertz, of the LVDS differential clock.
> +	 */
> +	unsigned long differential_clk_rate;
> +
> +	/**
> +	 * @lanes:
> +	 *
> +	 * Number of active, consecutive, data lanes, starting from
> +	 * lane 0, used for the transmissions.
> +	 */
> +	unsigned int lanes;
> +
> +	/**
> +	 * @is_slave:
> +	 *
> +	 * Boolean, true if the phy is a slave which works together
> +	 * with a master phy to support dual link transmission,
> +	 * otherwise a regular phy or a master phy.
> +	 */
> +	bool is_slave;
> +};
> +
> +#endif /* __PHY_LVDS_H_ */
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index e435bdb..d450b44 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -17,6 +17,7 @@
>  #include <linux/regulator/consumer.h>
>  
>  #include <linux/phy/phy-dp.h>
> +#include <linux/phy/phy-lvds.h>
>  #include <linux/phy/phy-mipi-dphy.h>
>  
>  struct phy;
> @@ -51,10 +52,13 @@ enum phy_mode {
>   *		the MIPI_DPHY phy mode.
>   * @dp:		Configuration set applicable for phys supporting
>   *		the DisplayPort protocol.
> + * @lvds:	Configuration set applicable for phys supporting
> + *		the LVDS phy mode.
>   */
>  union phy_configure_opts {
>  	struct phy_configure_opts_mipi_dphy	mipi_dphy;
>  	struct phy_configure_opts_dp		dp;
> +	struct phy_configure_opts_lvds		lvds;
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] phy: Add LVDS configuration options

[Liu Ying]

Hi Laurent,

On Tue, 2020-12-08 at 14:38 +0200, Laurent Pinchart wrote:
> Hi Liu,
> 
> Thank you for the patch.
> 
> On Fri, Dec 04, 2020 at 03:33:42PM +0800, Liu Ying wrote:
> > This patch allows LVDS PHYs to be configured through
> > the generic functions and through a custom structure
> > added to the generic union.
> > 
> > The parameters added here are based on common LVDS PHY
> > implementation practices.  The set of parameters
> > should cover all potential users.
> > 
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  include/linux/phy/phy-lvds.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/phy/phy.h      |  4 ++++
> >  2 files changed, 52 insertions(+)
> >  create mode 100644 include/linux/phy/phy-lvds.h
> > 
> > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h
> > new file mode 100644
> > index 00000000..1b5b9d6
> > --- /dev/null
> > +++ b/include/linux/phy/phy-lvds.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2020 NXP
> > + */
> > +
> > +#ifndef __PHY_LVDS_H_
> > +#define __PHY_LVDS_H_
> > +
> > +/**
> > + * struct phy_configure_opts_lvds - LVDS configuration set
> > + *
> > + * This structure is used to represent the configuration state of a
> > + * LVDS phy.
> > + */
> > +struct phy_configure_opts_lvds {
> > +	/**
> > +	 * @bits_per_lane_and_dclk_cycle:
> > +	 *
> > +	 * Number of bits per data lane and differential clock cycle.
> > +	 */
> > +	unsigned int bits_per_lane_and_dclk_cycle;
> 
> I see in patch 4/4 that you only support 7, can the value be any
> different ?

Patch 4/4 is for the Mixel combo PHY embedded in i.MX8qxp SoC.
This PHY can only do 7.

i.MX8qm SoC embeds another type of Mixel LVDS PHY which can do either 7
or 10(configurable with a register bit called 'NB').  A PHY driver for
it is yet to be upstreamed.

Regards,
Liu Ying

> 
> > +
> > +	/**
> > +	 * @differential_clk_rate:
> > +	 *
> > +	 * Clock rate, in Hertz, of the LVDS differential clock.
> > +	 */
> > +	unsigned long differential_clk_rate;
> > +
> > +	/**
> > +	 * @lanes:
> > +	 *
> > +	 * Number of active, consecutive, data lanes, starting from
> > +	 * lane 0, used for the transmissions.
> > +	 */
> > +	unsigned int lanes;
> > +
> > +	/**
> > +	 * @is_slave:
> > +	 *
> > +	 * Boolean, true if the phy is a slave which works together
> > +	 * with a master phy to support dual link transmission,
> > +	 * otherwise a regular phy or a master phy.
> > +	 */
> > +	bool is_slave;
> > +};
> > +
> > +#endif /* __PHY_LVDS_H_ */
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index e435bdb..d450b44 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/regulator/consumer.h>
> >  
> >  #include <linux/phy/phy-dp.h>
> > +#include <linux/phy/phy-lvds.h>
> >  #include <linux/phy/phy-mipi-dphy.h>
> >  
> >  struct phy;
> > @@ -51,10 +52,13 @@ enum phy_mode {
> >   *		the MIPI_DPHY phy mode.
> >   * @dp:		Configuration set applicable for phys supporting
> >   *		the DisplayPort protocol.
> > + * @lvds:	Configuration set applicable for phys supporting
> > + *		the LVDS phy mode.
> >   */
> >  union phy_configure_opts {
> >  	struct phy_configure_opts_mipi_dphy	mipi_dphy;
> >  	struct phy_configure_opts_dp		dp;
> > +	struct phy_configure_opts_lvds		lvds;
> >  };
> >  
> >  /**

[PATCH 3/4] dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for i.MX8qxp

[Liu Ying]

Add support for Mixel MIPI DPHY + LVDS PHY combo IP
as found on Freescale i.MX8qxp SoC.

Cc: Guido Günther <agx@sigxcpu.org>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
index 9b23407..0afce99 100644
--- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
+++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
@@ -4,9 +4,13 @@ The Mixel MIPI-DSI PHY IP block is e.g. found on i.MX8 platforms (along the
 MIPI-DSI IP from Northwest Logic). It represents the physical layer for the
 electrical signals for DSI.
 
+The Mixel PHY IP block found on i.MX8qxp is a combo PHY that can work
+in either MIPI-DSI PHY mode or LVDS PHY mode.
+
 Required properties:
-- compatible: Must be:
+- compatible: Should be one of:
   - "fsl,imx8mq-mipi-dphy"
+  - "fsl,imx8qxp-mipi-dphy"
 - clocks: Must contain an entry for each entry in clock-names.
 - clock-names: Must contain the following entries:
   - "phy_ref": phandle and specifier referring to the DPHY ref clock
@@ -14,6 +18,8 @@ Required properties:
 - #phy-cells: number of cells in PHY, as defined in
   Documentation/devicetree/bindings/phy/phy-bindings.txt
   this must be <0>
+- fsl,syscon: Phandle to a system controller, as required by the PHY
+  in i.MX8qxp SoC.
 
 Optional properties:
 - power-domains: phandle to power domain
-- 
2.7.4

Re: [PATCH 3/4] dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for i.MX8qxp

[Guido Günther]

Hi Liu,
Since we now gain optional properties validation would become even more
useful. Could you look into converting to YAML before adding more
values?
Cheers,
 -- Guido

On Fri, Dec 04, 2020 at 03:33:43PM +0800, Liu Ying wrote:
> Add support for Mixel MIPI DPHY + LVDS PHY combo IP
> as found on Freescale i.MX8qxp SoC.
> 
> Cc: Guido Günther <agx@sigxcpu.org>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> index 9b23407..0afce99 100644
> --- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> @@ -4,9 +4,13 @@ The Mixel MIPI-DSI PHY IP block is e.g. found on i.MX8 platforms (along the
>  MIPI-DSI IP from Northwest Logic). It represents the physical layer for the
>  electrical signals for DSI.
>  
> +The Mixel PHY IP block found on i.MX8qxp is a combo PHY that can work
> +in either MIPI-DSI PHY mode or LVDS PHY mode.
> +
>  Required properties:
> -- compatible: Must be:
> +- compatible: Should be one of:
>    - "fsl,imx8mq-mipi-dphy"
> +  - "fsl,imx8qxp-mipi-dphy"
>  - clocks: Must contain an entry for each entry in clock-names.
>  - clock-names: Must contain the following entries:
>    - "phy_ref": phandle and specifier referring to the DPHY ref clock
> @@ -14,6 +18,8 @@ Required properties:
>  - #phy-cells: number of cells in PHY, as defined in
>    Documentation/devicetree/bindings/phy/phy-bindings.txt
>    this must be <0>
> +- fsl,syscon: Phandle to a system controller, as required by the PHY
> +  in i.MX8qxp SoC.
>  
>  Optional properties:
>  - power-domains: phandle to power domain
> -- 
> 2.7.4
> 

Re: [PATCH 3/4] dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for i.MX8qxp

[Liu Ying]

On Tue, 2020-12-08 at 10:07 +0100, Guido Günther wrote:
> Hi Liu,
> Since we now gain optional properties validation would become even more
> useful. Could you look into converting to YAML before adding more
> values?

Yes, a YAML one would be good.
I'll try to do the conversion and then add the binding support for the
i.MX8qxp Mixel combo PHY in it.

Liu Ying

> Cheers,
>  -- Guido
> 
> On Fri, Dec 04, 2020 at 03:33:43PM +0800, Liu Ying wrote:
> > Add support for Mixel MIPI DPHY + LVDS PHY combo IP
> > as found on Freescale i.MX8qxp SoC.
> > 
> > Cc: Guido Günther <agx@sigxcpu.org>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> > index 9b23407..0afce99 100644
> > --- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
> > @@ -4,9 +4,13 @@ The Mixel MIPI-DSI PHY IP block is e.g. found on i.MX8 platforms (along the
> >  MIPI-DSI IP from Northwest Logic). It represents the physical layer for the
> >  electrical signals for DSI.
> >  
> > +The Mixel PHY IP block found on i.MX8qxp is a combo PHY that can work
> > +in either MIPI-DSI PHY mode or LVDS PHY mode.
> > +
> >  Required properties:
> > -- compatible: Must be:
> > +- compatible: Should be one of:
> >    - "fsl,imx8mq-mipi-dphy"
> > +  - "fsl,imx8qxp-mipi-dphy"
> >  - clocks: Must contain an entry for each entry in clock-names.
> >  - clock-names: Must contain the following entries:
> >    - "phy_ref": phandle and specifier referring to the DPHY ref clock
> > @@ -14,6 +18,8 @@ Required properties:
> >  - #phy-cells: number of cells in PHY, as defined in
> >    Documentation/devicetree/bindings/phy/phy-bindings.txt
> >    this must be <0>
> > +- fsl,syscon: Phandle to a system controller, as required by the PHY
> > +  in i.MX8qxp SoC.
> >  
> >  Optional properties:
> >  - power-domains: phandle to power domain
> > -- 
> > 2.7.4
> > 

[PATCH 4/4] phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Liu Ying]

i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which supports
either a MIPI DSI display or a LVDS display.  The PHY mode is controlled
by SCU firmware and the driver would call a SCU firmware function to
configure the PHY mode.  The single LVDS PHY has 4 data lanes to support
a LVDS display.  Also, with a master LVDS PHY and a slave LVDS PHY, they
may work together to support a LVDS display with 8 data lanes(usually, dual
LVDS link display).  Note that this patch supports the LVDS PHY mode only
for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet to be
supported, so for now error would be returned from ->set_mode() if MIPI
DPHY mode is passed over to it for the combo PHY.

Cc: Guido Günther <agx@sigxcpu.org>
Cc: Robert Chiras <robert.chiras@nxp.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 266 ++++++++++++++++++++++++-
 1 file changed, 255 insertions(+), 11 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
index a95572b..37084a9 100644
--- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
+++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
@@ -4,17 +4,31 @@
  * Copyright 2019 Purism SPC
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/delay.h>
+#include <linux/firmware/imx/ipc.h>
+#include <linux/firmware/imx/svc/misc.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <dt-bindings/firmware/imx/rsrc.h>
+
+/* Control and Status Registers(CSR) */
+#define PHY_CTRL			0x00
+#define  CCM_MASK			GENMASK(7, 5)
+#define  CCM(n)				FIELD_PREP(CCM_MASK, (n))
+#define  CA_MASK			GENMASK(4, 2)
+#define  CA(n)				FIELD_PREP(CA_MASK, (n))
+#define  RFB				BIT(1)
+#define  LVDS_EN			BIT(0)
 
 /* DPHY registers */
 #define DPHY_PD_DPHY			0x00
@@ -55,8 +69,15 @@
 #define PWR_ON	0
 #define PWR_OFF	1
 
+#define MIN_VCO_FREQ 640000000
+#define MAX_VCO_FREQ 1500000000
+
+#define MIN_LVDS_REFCLK_FREQ 24000000
+#define MAX_LVDS_REFCLK_FREQ 150000000
+
 enum mixel_dphy_devtype {
 	MIXEL_IMX8MQ,
+	MIXEL_IMX8QXP,
 };
 
 struct mixel_dphy_devdata {
@@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
 	u8 reg_rxlprp;
 	u8 reg_rxcdrp;
 	u8 reg_rxhs_settle;
+	bool is_combo;	/* MIPI DPHY and LVDS PHY combo */
 };
 
 static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
@@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
 		.reg_rxlprp = 0x40,
 		.reg_rxcdrp = 0x44,
 		.reg_rxhs_settle = 0x48,
+		.is_combo = false,
+	},
+	[MIXEL_IMX8QXP] = {
+		.is_combo = true,
 	},
 };
 
@@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
 struct mixel_dphy_priv {
 	struct mixel_dphy_cfg cfg;
 	struct regmap *regmap;
+	struct regmap *lvds_regmap;
 	struct clk *phy_ref_clk;
 	const struct mixel_dphy_devdata *devdata;
+	struct imx_sc_ipc *ipc_handle;
+	bool is_slave;
+	int id;
 };
 
 static const struct regmap_config mixel_dphy_regmap_config = {
@@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy *phy)
 	return 0;
 }
 
-static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
+static int
+mixel_dphy_configure_mipi_dphy(struct phy *phy, union phy_configure_opts *opts)
 {
 	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
 	struct mixel_dphy_cfg cfg = { 0 };
@@ -345,15 +376,118 @@ static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 	return 0;
 }
 
+static int
+mixel_dphy_configure_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
+	unsigned long data_rate;
+	unsigned long fvco;
+	u32 rsc;
+	u32 co;
+	int ret;
+
+	priv->is_slave = lvds_opts->is_slave;
+
+	/* LVDS interface pins */
+	regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) | RFB);
+
+	/* enable MODE8 only for slave LVDS PHY */
+	rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
+	ret = imx_sc_misc_set_control(priv->ipc_handle, rsc, IMX_SC_C_DUAL_MODE,
+				      lvds_opts->is_slave);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to configure MODE8: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Choose an appropriate divider ratio to meet the requirement of
+	 * PLL VCO frequency range.
+	 *
+	 *  -----  640MHz ~ 1500MHz   ------------      ---------------
+	 * | VCO | ----------------> | CO divider | -> | LVDS data rate|
+	 *  -----       FVCO          ------------      ---------------
+	 *                            1/2/4/8 div     7 * differential_clk_rate
+	 */
+	data_rate = 7 * lvds_opts->differential_clk_rate;
+	for (co = 1; co <= 8; co *= 2) {
+		fvco = data_rate * co;
+
+		if (fvco >= MIN_VCO_FREQ)
+			break;
+	}
+
+	if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
+		dev_err(&phy->dev, "VCO frequency %lu is out of range\n", fvco);
+		return -ERANGE;
+	}
+
+	/*
+	 * CO is configurable, while CN and CM are not,
+	 * as fixed ratios 1 and 7 are applied respectively.
+	 */
+	phy_write(phy, __ffs(co), DPHY_CO);
+
+	/* set reference clock rate */
+	clk_set_rate(priv->phy_ref_clk, lvds_opts->differential_clk_rate);
+
+	return ret;
+}
+
+static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
+{
+	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
+		return mixel_dphy_configure_mipi_dphy(phy, opts);
+	else if (phy->attrs.mode == PHY_MODE_LVDS)
+		return mixel_dphy_configure_lvds_phy(phy, opts);
+
+	dev_err(&phy->dev, "Failed to configure PHY with invalid PHY mode\n");
+	return -EINVAL;
+}
+
+static int
+mixel_dphy_validate_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
+{
+	struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
+
+	if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
+		dev_err(&phy->dev, "Invalid bits per LVDS data lane: %u\n",
+					lvds_cfg->bits_per_lane_and_dclk_cycle);
+		return -EINVAL;
+	}
+
+	if (lvds_cfg->lanes != 4) {
+		dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
+						lvds_cfg->lanes);
+		return -EINVAL;
+	}
+
+	if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
+	    lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
+		dev_err(&phy->dev,
+			"Invalid LVDS differential clock rate: %lu\n",
+					lvds_cfg->differential_clk_rate);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
 			       union phy_configure_opts *opts)
 {
-	struct mixel_dphy_cfg cfg = { 0 };
+	if (mode == PHY_MODE_MIPI_DPHY) {
+		struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
 
-	if (mode != PHY_MODE_MIPI_DPHY)
-		return -EINVAL;
+		return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
+							&mipi_dphy_cfg);
+	} else if (mode == PHY_MODE_LVDS) {
+		return mixel_dphy_validate_lvds_phy(phy, opts);
+	}
 
-	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+	dev_err(&phy->dev, "Failed to validate PHY with invalid PHY mode\n");
+	return -EINVAL;
 }
 
 static int mixel_dphy_init(struct phy *phy)
@@ -373,27 +507,74 @@ static int mixel_dphy_exit(struct phy *phy)
 	return 0;
 }
 
-static int mixel_dphy_power_on(struct phy *phy)
+static int mixel_dphy_power_on_mipi_dphy(struct phy *phy)
 {
 	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
 	u32 locked;
 	int ret;
 
-	ret = clk_prepare_enable(priv->phy_ref_clk);
-	if (ret < 0)
-		return ret;
-
 	phy_write(phy, PWR_ON, DPHY_PD_PLL);
 	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
 				       locked, PLL_LOCK_SLEEP,
 				       PLL_LOCK_TIMEOUT);
 	if (ret < 0) {
 		dev_err(&phy->dev, "Could not get DPHY lock (%d)!\n", ret);
-		goto clock_disable;
+		return ret;
 	}
 	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
 
 	return 0;
+}
+
+static int mixel_dphy_power_on_lvds_phy(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	u32 locked;
+	int ret;
+
+	regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, LVDS_EN);
+
+	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
+	phy_write(phy, PWR_ON, DPHY_PD_PLL);
+
+	/* do not wait for slave LVDS PHY being locked */
+	if (priv->is_slave)
+		return 0;
+
+	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
+				       locked, PLL_LOCK_SLEEP,
+				       PLL_LOCK_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&phy->dev, "Could not get LVDS PHY lock (%d)!\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mixel_dphy_power_on(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = clk_prepare_enable(priv->phy_ref_clk);
+	if (ret < 0)
+		return ret;
+
+	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY) {
+		ret = mixel_dphy_power_on_mipi_dphy(phy);
+	} else if (phy->attrs.mode == PHY_MODE_LVDS) {
+		ret = mixel_dphy_power_on_lvds_phy(phy);
+	} else {
+		dev_err(&phy->dev,
+			"Failed to power on PHY with invalid PHY mode\n");
+		ret = -EINVAL;
+	}
+
+	if (ret)
+		goto clock_disable;
+
+	return 0;
 clock_disable:
 	clk_disable_unprepare(priv->phy_ref_clk);
 	return ret;
@@ -406,16 +587,52 @@ static int mixel_dphy_power_off(struct phy *phy)
 	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
 	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
 
+	if (phy->attrs.mode == PHY_MODE_LVDS)
+		regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, 0);
+
 	clk_disable_unprepare(priv->phy_ref_clk);
 
 	return 0;
 }
 
+static int mixel_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	/* Currently, MIPI DPHY mode only, if it's not a combo PHY. */
+	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
+		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
+		return -EINVAL;
+	}
+
+	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
+		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
+		return -EINVAL;
+	}
+
+	if (priv->devdata->is_combo) {
+		u32 rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
+
+		ret = imx_sc_misc_set_control(priv->ipc_handle,
+					      rsc, IMX_SC_C_MODE,
+					      mode == PHY_MODE_LVDS);
+		if (ret) {
+			dev_err(&phy->dev,
+				"Failed to set PHY mode via SCU ipc: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static const struct phy_ops mixel_dphy_phy_ops = {
 	.init = mixel_dphy_init,
 	.exit = mixel_dphy_exit,
 	.power_on = mixel_dphy_power_on,
 	.power_off = mixel_dphy_power_off,
+	.set_mode = mixel_dphy_set_mode,
 	.configure = mixel_dphy_configure,
 	.validate = mixel_dphy_validate,
 	.owner = THIS_MODULE,
@@ -424,6 +641,8 @@ static const struct phy_ops mixel_dphy_phy_ops = {
 static const struct of_device_id mixel_dphy_of_match[] = {
 	{ .compatible = "fsl,imx8mq-mipi-dphy",
 	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
+	{ .compatible = "fsl,imx8qxp-mipi-dphy",
+	  .data = &mixel_dphy_devdata[MIXEL_IMX8QXP] },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
@@ -436,6 +655,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
 	struct mixel_dphy_priv *priv;
 	struct phy *phy;
 	void __iomem *base;
+	int ret;
 
 	if (!np)
 		return -ENODEV;
@@ -467,6 +687,30 @@ static int mixel_dphy_probe(struct platform_device *pdev)
 	dev_dbg(dev, "phy_ref clock rate: %lu\n",
 		clk_get_rate(priv->phy_ref_clk));
 
+	if (priv->devdata->is_combo) {
+		priv->lvds_regmap =
+			syscon_regmap_lookup_by_phandle(np, "fsl,syscon");
+		if (IS_ERR(priv->lvds_regmap)) {
+			ret = PTR_ERR(priv->lvds_regmap);
+			dev_err_probe(dev, ret, "Failed to get LVDS regmap\n");
+			return ret;
+		}
+
+		priv->id = of_alias_get_id(np, "mipi_dphy");
+		if (priv->id < 0) {
+			dev_err(dev, "Failed to get phy node alias id: %d\n",
+								 priv->id);
+			return priv->id;
+		}
+
+		ret = imx_scu_get_handle(&priv->ipc_handle);
+		if (ret) {
+			dev_err_probe(dev, ret,
+				      "Failed to get SCU ipc handle\n");
+			return ret;
+		}
+	}
+
 	dev_set_drvdata(dev, priv);
 
 	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
-- 
2.7.4

Re: [PATCH 4/4] phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Guido Günther]

Hi Liu,
some minor comments inline:

On Fri, Dec 04, 2020 at 03:33:44PM +0800, Liu Ying wrote:
> i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which supports
> either a MIPI DSI display or a LVDS display.  The PHY mode is controlled
> by SCU firmware and the driver would call a SCU firmware function to
> configure the PHY mode.  The single LVDS PHY has 4 data lanes to support
> a LVDS display.  Also, with a master LVDS PHY and a slave LVDS PHY, they
> may work together to support a LVDS display with 8 data lanes(usually, dual
> LVDS link display).  Note that this patch supports the LVDS PHY mode only
> for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet to be
> supported, so for now error would be returned from ->set_mode() if MIPI
> DPHY mode is passed over to it for the combo PHY.
> 
> Cc: Guido Günther <agx@sigxcpu.org>
> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 266 ++++++++++++++++++++++++-
>  1 file changed, 255 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> index a95572b..37084a9 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> @@ -4,17 +4,31 @@
>   * Copyright 2019 Purism SPC
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/delay.h>
> +#include <linux/firmware/imx/ipc.h>
> +#include <linux/firmware/imx/svc/misc.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <dt-bindings/firmware/imx/rsrc.h>
> +
> +/* Control and Status Registers(CSR) */
> +#define PHY_CTRL			0x00
> +#define  CCM_MASK			GENMASK(7, 5)
> +#define  CCM(n)				FIELD_PREP(CCM_MASK, (n))
> +#define  CA_MASK			GENMASK(4, 2)
> +#define  CA(n)				FIELD_PREP(CA_MASK, (n))
> +#define  RFB				BIT(1)
> +#define  LVDS_EN			BIT(0)
>  
>  /* DPHY registers */
>  #define DPHY_PD_DPHY			0x00
> @@ -55,8 +69,15 @@
>  #define PWR_ON	0
>  #define PWR_OFF	1
>  
> +#define MIN_VCO_FREQ 640000000
> +#define MAX_VCO_FREQ 1500000000
> +
> +#define MIN_LVDS_REFCLK_FREQ 24000000
> +#define MAX_LVDS_REFCLK_FREQ 150000000
> +
>  enum mixel_dphy_devtype {
>  	MIXEL_IMX8MQ,
> +	MIXEL_IMX8QXP,
>  };
>  
>  struct mixel_dphy_devdata {
> @@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
>  	u8 reg_rxlprp;
>  	u8 reg_rxcdrp;
>  	u8 reg_rxhs_settle;
> +	bool is_combo;	/* MIPI DPHY and LVDS PHY combo */
>  };
>  
>  static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> @@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
>  		.reg_rxlprp = 0x40,
>  		.reg_rxcdrp = 0x44,
>  		.reg_rxhs_settle = 0x48,
> +		.is_combo = false,
> +	},
> +	[MIXEL_IMX8QXP] = {
> +		.is_combo = true,
>  	},
>  };
>  
> @@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
>  struct mixel_dphy_priv {
>  	struct mixel_dphy_cfg cfg;
>  	struct regmap *regmap;
> +	struct regmap *lvds_regmap;
>  	struct clk *phy_ref_clk;
>  	const struct mixel_dphy_devdata *devdata;
> +	struct imx_sc_ipc *ipc_handle;
> +	bool is_slave;
> +	int id;
>  };
>  
>  static const struct regmap_config mixel_dphy_regmap_config = {
> @@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy *phy)
>  	return 0;
>  }
>  
> -static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> +static int
> +mixel_dphy_configure_mipi_dphy(struct phy *phy, union phy_configure_opts *opts)
>  {
>  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
>  	struct mixel_dphy_cfg cfg = { 0 };
> @@ -345,15 +376,118 @@ static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	return 0;
>  }
>  
> +static int
> +mixel_dphy_configure_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
> +	unsigned long data_rate;
> +	unsigned long fvco;
> +	u32 rsc;
> +	u32 co;
> +	int ret;
> +
> +	priv->is_slave = lvds_opts->is_slave;
> +
> +	/* LVDS interface pins */
> +	regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) | RFB);
> +
> +	/* enable MODE8 only for slave LVDS PHY */
> +	rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> +	ret = imx_sc_misc_set_control(priv->ipc_handle, rsc, IMX_SC_C_DUAL_MODE,
> +				      lvds_opts->is_slave);
> +	if (ret) {
> +		dev_err(&phy->dev, "Failed to configure MODE8: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Choose an appropriate divider ratio to meet the requirement of
> +	 * PLL VCO frequency range.
> +	 *
> +	 *  -----  640MHz ~ 1500MHz   ------------      ---------------
> +	 * | VCO | ----------------> | CO divider | -> | LVDS data rate|
> +	 *  -----       FVCO          ------------      ---------------
> +	 *                            1/2/4/8 div     7 * differential_clk_rate
> +	 */
> +	data_rate = 7 * lvds_opts->differential_clk_rate;
> +	for (co = 1; co <= 8; co *= 2) {
> +		fvco = data_rate * co;
> +
> +		if (fvco >= MIN_VCO_FREQ)
> +			break;
> +	}
> +
> +	if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
> +		dev_err(&phy->dev, "VCO frequency %lu is out of range\n", fvco);
> +		return -ERANGE;
> +	}
> +
> +	/*
> +	 * CO is configurable, while CN and CM are not,
> +	 * as fixed ratios 1 and 7 are applied respectively.
> +	 */
> +	phy_write(phy, __ffs(co), DPHY_CO);
> +
> +	/* set reference clock rate */
> +	clk_set_rate(priv->phy_ref_clk, lvds_opts->differential_clk_rate);
> +
> +	return ret;
> +}
> +
> +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
> +		return mixel_dphy_configure_mipi_dphy(phy, opts);
> +	else if (phy->attrs.mode == PHY_MODE_LVDS)
> +		return mixel_dphy_configure_lvds_phy(phy, opts);
> +
> +	dev_err(&phy->dev, "Failed to configure PHY with invalid PHY mode\n");
> +	return -EINVAL;
> +}
> +
> +static int
> +mixel_dphy_validate_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> +{
> +	struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
> +
> +	if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
> +		dev_err(&phy->dev, "Invalid bits per LVDS data lane: %u\n",
> +					lvds_cfg->bits_per_lane_and_dclk_cycle);
> +		return -EINVAL;
> +	}
> +
> +	if (lvds_cfg->lanes != 4) {
> +		dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
> +						lvds_cfg->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
> +	    lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
> +		dev_err(&phy->dev,
> +			"Invalid LVDS differential clock rate: %lu\n",
> +					lvds_cfg->differential_clk_rate);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  			       union phy_configure_opts *opts)
>  {
> -	struct mixel_dphy_cfg cfg = { 0 };
> +	if (mode == PHY_MODE_MIPI_DPHY) {
> +		struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
>  
> -	if (mode != PHY_MODE_MIPI_DPHY)
> -		return -EINVAL;
> +		return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> +							&mipi_dphy_cfg);
> +	} else if (mode == PHY_MODE_LVDS) {
> +		return mixel_dphy_validate_lvds_phy(phy, opts);
> +	}
>  
> -	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> +	dev_err(&phy->dev, "Failed to validate PHY with invalid PHY mode\n");

Can you print the `mode` here so it becomes obvious from the error what
incorrect mode got passed in?

> +	return -EINVAL;
>  }
>  
>  static int mixel_dphy_init(struct phy *phy)
> @@ -373,27 +507,74 @@ static int mixel_dphy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> -static int mixel_dphy_power_on(struct phy *phy)
> +static int mixel_dphy_power_on_mipi_dphy(struct phy *phy)
>  {
>  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
>  	u32 locked;
>  	int ret;
>  
> -	ret = clk_prepare_enable(priv->phy_ref_clk);
> -	if (ret < 0)
> -		return ret;
> -
>  	phy_write(phy, PWR_ON, DPHY_PD_PLL);
>  	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
>  				       locked, PLL_LOCK_SLEEP,
>  				       PLL_LOCK_TIMEOUT);
>  	if (ret < 0) {
>  		dev_err(&phy->dev, "Could not get DPHY lock (%d)!\n", ret);
> -		goto clock_disable;
> +		return ret;
>  	}
>  	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
>  
>  	return 0;
> +}
> +
> +static int mixel_dphy_power_on_lvds_phy(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	u32 locked;
> +	int ret;
> +
> +	regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, LVDS_EN);
> +
> +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> +
> +	/* do not wait for slave LVDS PHY being locked */
> +	if (priv->is_slave)
> +		return 0;
> +
> +	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> +				       locked, PLL_LOCK_SLEEP,
> +				       PLL_LOCK_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&phy->dev, "Could not get LVDS PHY lock (%d)!\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mixel_dphy_power_on(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->phy_ref_clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY) {
> +		ret = mixel_dphy_power_on_mipi_dphy(phy);
> +	} else if (phy->attrs.mode == PHY_MODE_LVDS) {
> +		ret = mixel_dphy_power_on_lvds_phy(phy);
> +	} else {
> +		dev_err(&phy->dev,
> +			"Failed to power on PHY with invalid PHY mode\n");

Can you print the `mode` here so it becomes obvious from the error what
incorrect mode got passed in?

> +		ret = -EINVAL;
> +	}
> +
> +	if (ret)
> +		goto clock_disable;
> +
> +	return 0;
>  clock_disable:
>  	clk_disable_unprepare(priv->phy_ref_clk);
>  	return ret;
> @@ -406,16 +587,52 @@ static int mixel_dphy_power_off(struct phy *phy)
>  	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
>  	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
>  
> +	if (phy->attrs.mode == PHY_MODE_LVDS)
> +		regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, 0);
> +
>  	clk_disable_unprepare(priv->phy_ref_clk);
>  
>  	return 0;
>  }
>  
> +static int mixel_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	int ret;
> +

I'd reject all modes except PHY_MODE_MIPI_DPHY and PHY_MODE_MIPI_LVDS upfront...

> +	/* Currently, MIPI DPHY mode only, if it's not a combo PHY. */
> +	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
> +		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
> +		return -EINVAL;
> +	}
> +
> +	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
> +		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
> +		return -EINVAL;
> +	}

...and then just reject on whetehr `is_combo` is set or not. This makes
it easier to add more later.

> +	if (priv->devdata->is_combo) {
> +		u32 rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> +
> +		ret = imx_sc_misc_set_control(priv->ipc_handle,
> +					      rsc, IMX_SC_C_MODE,
> +					      mode == PHY_MODE_LVDS);
> +		if (ret) {
> +			dev_err(&phy->dev,
> +				"Failed to set PHY mode via SCU ipc: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct phy_ops mixel_dphy_phy_ops = {
>  	.init = mixel_dphy_init,
>  	.exit = mixel_dphy_exit,
>  	.power_on = mixel_dphy_power_on,
>  	.power_off = mixel_dphy_power_off,
> +	.set_mode = mixel_dphy_set_mode,
>  	.configure = mixel_dphy_configure,
>  	.validate = mixel_dphy_validate,
>  	.owner = THIS_MODULE,
> @@ -424,6 +641,8 @@ static const struct phy_ops mixel_dphy_phy_ops = {
>  static const struct of_device_id mixel_dphy_of_match[] = {
>  	{ .compatible = "fsl,imx8mq-mipi-dphy",
>  	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> +	{ .compatible = "fsl,imx8qxp-mipi-dphy",
> +	  .data = &mixel_dphy_devdata[MIXEL_IMX8QXP] },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> @@ -436,6 +655,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
>  	struct mixel_dphy_priv *priv;
>  	struct phy *phy;
>  	void __iomem *base;
> +	int ret;
>  
>  	if (!np)
>  		return -ENODEV;
> @@ -467,6 +687,30 @@ static int mixel_dphy_probe(struct platform_device *pdev)
>  	dev_dbg(dev, "phy_ref clock rate: %lu\n",
>  		clk_get_rate(priv->phy_ref_clk));
>  
> +	if (priv->devdata->is_combo) {
> +		priv->lvds_regmap =
> +			syscon_regmap_lookup_by_phandle(np, "fsl,syscon");
> +		if (IS_ERR(priv->lvds_regmap)) {
> +			ret = PTR_ERR(priv->lvds_regmap);
> +			dev_err_probe(dev, ret, "Failed to get LVDS regmap\n");
> +			return ret;
> +		}
> +
> +		priv->id = of_alias_get_id(np, "mipi_dphy");
> +		if (priv->id < 0) {
> +			dev_err(dev, "Failed to get phy node alias id: %d\n",
> +								 priv->id);
> +			return priv->id;
> +		}
> +
> +		ret = imx_scu_get_handle(&priv->ipc_handle);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to get SCU ipc handle\n");
> +			return ret;
> +		}
> +	}
> +
>  	dev_set_drvdata(dev, priv);
>  
>  	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> -- 
> 2.7.4
> 

Cheers,
 -- Guido

Re: [PATCH 4/4] phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Liu Ying]

On Tue, 2020-12-08 at 10:24 +0100, Guido Günther wrote:
> Hi Liu,
> some minor comments inline:
> 
> On Fri, Dec 04, 2020 at 03:33:44PM +0800, Liu Ying wrote:
> > i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which supports
> > either a MIPI DSI display or a LVDS display.  The PHY mode is controlled
> > by SCU firmware and the driver would call a SCU firmware function to
> > configure the PHY mode.  The single LVDS PHY has 4 data lanes to support
> > a LVDS display.  Also, with a master LVDS PHY and a slave LVDS PHY, they
> > may work together to support a LVDS display with 8 data lanes(usually, dual
> > LVDS link display).  Note that this patch supports the LVDS PHY mode only
> > for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet to be
> > supported, so for now error would be returned from ->set_mode() if MIPI
> > DPHY mode is passed over to it for the combo PHY.
> > 
> > Cc: Guido Günther <agx@sigxcpu.org>
> > Cc: Robert Chiras <robert.chiras@nxp.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 266 ++++++++++++++++++++++++-
> >  1 file changed, 255 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > index a95572b..37084a9 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > @@ -4,17 +4,31 @@
> >   * Copyright 2019 Purism SPC
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/delay.h>
> > +#include <linux/firmware/imx/ipc.h>
> > +#include <linux/firmware/imx/svc/misc.h>
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> > +#include <dt-bindings/firmware/imx/rsrc.h>
> > +
> > +/* Control and Status Registers(CSR) */
> > +#define PHY_CTRL			0x00
> > +#define  CCM_MASK			GENMASK(7, 5)
> > +#define  CCM(n)				FIELD_PREP(CCM_MASK, (n))
> > +#define  CA_MASK			GENMASK(4, 2)
> > +#define  CA(n)				FIELD_PREP(CA_MASK, (n))
> > +#define  RFB				BIT(1)
> > +#define  LVDS_EN			BIT(0)
> >  
> >  /* DPHY registers */
> >  #define DPHY_PD_DPHY			0x00
> > @@ -55,8 +69,15 @@
> >  #define PWR_ON	0
> >  #define PWR_OFF	1
> >  
> > +#define MIN_VCO_FREQ 640000000
> > +#define MAX_VCO_FREQ 1500000000
> > +
> > +#define MIN_LVDS_REFCLK_FREQ 24000000
> > +#define MAX_LVDS_REFCLK_FREQ 150000000
> > +
> >  enum mixel_dphy_devtype {
> >  	MIXEL_IMX8MQ,
> > +	MIXEL_IMX8QXP,
> >  };
> >  
> >  struct mixel_dphy_devdata {
> > @@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
> >  	u8 reg_rxlprp;
> >  	u8 reg_rxcdrp;
> >  	u8 reg_rxhs_settle;
> > +	bool is_combo;	/* MIPI DPHY and LVDS PHY combo */
> >  };
> >  
> >  static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > @@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> >  		.reg_rxlprp = 0x40,
> >  		.reg_rxcdrp = 0x44,
> >  		.reg_rxhs_settle = 0x48,
> > +		.is_combo = false,
> > +	},
> > +	[MIXEL_IMX8QXP] = {
> > +		.is_combo = true,
> >  	},
> >  };
> >  
> > @@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
> >  struct mixel_dphy_priv {
> >  	struct mixel_dphy_cfg cfg;
> >  	struct regmap *regmap;
> > +	struct regmap *lvds_regmap;
> >  	struct clk *phy_ref_clk;
> >  	const struct mixel_dphy_devdata *devdata;
> > +	struct imx_sc_ipc *ipc_handle;
> > +	bool is_slave;
> > +	int id;
> >  };
> >  
> >  static const struct regmap_config mixel_dphy_regmap_config = {
> > @@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy *phy)
> >  	return 0;
> >  }
> >  
> > -static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > +static int
> > +mixel_dphy_configure_mipi_dphy(struct phy *phy, union phy_configure_opts *opts)
> >  {
> >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> >  	struct mixel_dphy_cfg cfg = { 0 };
> > @@ -345,15 +376,118 @@ static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> >  	return 0;
> >  }
> >  
> > +static int
> > +mixel_dphy_configure_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
> > +	unsigned long data_rate;
> > +	unsigned long fvco;
> > +	u32 rsc;
> > +	u32 co;
> > +	int ret;
> > +
> > +	priv->is_slave = lvds_opts->is_slave;
> > +
> > +	/* LVDS interface pins */
> > +	regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) | RFB);
> > +
> > +	/* enable MODE8 only for slave LVDS PHY */
> > +	rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > +	ret = imx_sc_misc_set_control(priv->ipc_handle, rsc, IMX_SC_C_DUAL_MODE,
> > +				      lvds_opts->is_slave);
> > +	if (ret) {
> > +		dev_err(&phy->dev, "Failed to configure MODE8: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Choose an appropriate divider ratio to meet the requirement of
> > +	 * PLL VCO frequency range.
> > +	 *
> > +	 *  -----  640MHz ~ 1500MHz   ------------      ---------------
> > +	 * | VCO | ----------------> | CO divider | -> | LVDS data rate|
> > +	 *  -----       FVCO          ------------      ---------------
> > +	 *                            1/2/4/8 div     7 * differential_clk_rate
> > +	 */
> > +	data_rate = 7 * lvds_opts->differential_clk_rate;
> > +	for (co = 1; co <= 8; co *= 2) {
> > +		fvco = data_rate * co;
> > +
> > +		if (fvco >= MIN_VCO_FREQ)
> > +			break;
> > +	}
> > +
> > +	if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
> > +		dev_err(&phy->dev, "VCO frequency %lu is out of range\n", fvco);
> > +		return -ERANGE;
> > +	}
> > +
> > +	/*
> > +	 * CO is configurable, while CN and CM are not,
> > +	 * as fixed ratios 1 and 7 are applied respectively.
> > +	 */
> > +	phy_write(phy, __ffs(co), DPHY_CO);
> > +
> > +	/* set reference clock rate */
> > +	clk_set_rate(priv->phy_ref_clk, lvds_opts->differential_clk_rate);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
> > +		return mixel_dphy_configure_mipi_dphy(phy, opts);
> > +	else if (phy->attrs.mode == PHY_MODE_LVDS)
> > +		return mixel_dphy_configure_lvds_phy(phy, opts);
> > +
> > +	dev_err(&phy->dev, "Failed to configure PHY with invalid PHY mode\n");
> > +	return -EINVAL;
> > +}
> > +
> > +static int
> > +mixel_dphy_validate_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +	struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
> > +
> > +	if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
> > +		dev_err(&phy->dev, "Invalid bits per LVDS data lane: %u\n",
> > +					lvds_cfg->bits_per_lane_and_dclk_cycle);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (lvds_cfg->lanes != 4) {
> > +		dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
> > +						lvds_cfg->lanes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
> > +	    lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
> > +		dev_err(&phy->dev,
> > +			"Invalid LVDS differential clock rate: %lu\n",
> > +					lvds_cfg->differential_clk_rate);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  			       union phy_configure_opts *opts)
> >  {
> > -	struct mixel_dphy_cfg cfg = { 0 };
> > +	if (mode == PHY_MODE_MIPI_DPHY) {
> > +		struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
> >  
> > -	if (mode != PHY_MODE_MIPI_DPHY)
> > -		return -EINVAL;
> > +		return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > +							&mipi_dphy_cfg);
> > +	} else if (mode == PHY_MODE_LVDS) {
> > +		return mixel_dphy_validate_lvds_phy(phy, opts);
> > +	}
> >  
> > -	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > +	dev_err(&phy->dev, "Failed to validate PHY with invalid PHY mode\n");
> 
> Can you print the `mode` here so it becomes obvious from the error what
> incorrect mode got passed in?

Will do in the next version.

> 
> > +	return -EINVAL;
> >  }
> >  
> >  static int mixel_dphy_init(struct phy *phy)
> > @@ -373,27 +507,74 @@ static int mixel_dphy_exit(struct phy *phy)
> >  	return 0;
> >  }
> >  
> > -static int mixel_dphy_power_on(struct phy *phy)
> > +static int mixel_dphy_power_on_mipi_dphy(struct phy *phy)
> >  {
> >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> >  	u32 locked;
> >  	int ret;
> >  
> > -	ret = clk_prepare_enable(priv->phy_ref_clk);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> >  	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> >  				       locked, PLL_LOCK_SLEEP,
> >  				       PLL_LOCK_TIMEOUT);
> >  	if (ret < 0) {
> >  		dev_err(&phy->dev, "Could not get DPHY lock (%d)!\n", ret);
> > -		goto clock_disable;
> > +		return ret;
> >  	}
> >  	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> >  
> >  	return 0;
> > +}
> > +
> > +static int mixel_dphy_power_on_lvds_phy(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	u32 locked;
> > +	int ret;
> > +
> > +	regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, LVDS_EN);
> > +
> > +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > +
> > +	/* do not wait for slave LVDS PHY being locked */
> > +	if (priv->is_slave)
> > +		return 0;
> > +
> > +	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> > +				       locked, PLL_LOCK_SLEEP,
> > +				       PLL_LOCK_TIMEOUT);
> > +	if (ret < 0) {
> > +		dev_err(&phy->dev, "Could not get LVDS PHY lock (%d)!\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mixel_dphy_power_on(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(priv->phy_ref_clk);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY) {
> > +		ret = mixel_dphy_power_on_mipi_dphy(phy);
> > +	} else if (phy->attrs.mode == PHY_MODE_LVDS) {
> > +		ret = mixel_dphy_power_on_lvds_phy(phy);
> > +	} else {
> > +		dev_err(&phy->dev,
> > +			"Failed to power on PHY with invalid PHY mode\n");
> 
> Can you print the `mode` here so it becomes obvious from the error what
> incorrect mode got passed in?

Will do in the next version.

> 
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	if (ret)
> > +		goto clock_disable;
> > +
> > +	return 0;
> >  clock_disable:
> >  	clk_disable_unprepare(priv->phy_ref_clk);
> >  	return ret;
> > @@ -406,16 +587,52 @@ static int mixel_dphy_power_off(struct phy *phy)
> >  	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> >  	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> >  
> > +	if (phy->attrs.mode == PHY_MODE_LVDS)
> > +		regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, 0);
> > +
> >  	clk_disable_unprepare(priv->phy_ref_clk);
> >  
> >  	return 0;
> >  }
> >  
> > +static int mixel_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	int ret;
> > +
> 
> I'd reject all modes except PHY_MODE_MIPI_DPHY and PHY_MODE_MIPI_LVDS upfront...

It seems that this overall rejection would cause _double_ check
together with the snippet right below, which should be avoided.

> 
> > +	/* Currently, MIPI DPHY mode only, if it's not a combo PHY. */
> > +	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
> > +		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
> > +		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
> > +		return -EINVAL;
> > +	}
> 
> ...and then just reject on whetehr `is_combo` is set or not. This makes
> it easier to add more later.

The above snippet is rejecting on whether 'is_combo' is set or not.
So, in short, the current implementation looks ok to me.
Or, things still can be improved here?

Thanks,
Liu Ying

> 
> > +	if (priv->devdata->is_combo) {
> > +		u32 rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > +
> > +		ret = imx_sc_misc_set_control(priv->ipc_handle,
> > +					      rsc, IMX_SC_C_MODE,
> > +					      mode == PHY_MODE_LVDS);
> > +		if (ret) {
> > +			dev_err(&phy->dev,
> > +				"Failed to set PHY mode via SCU ipc: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct phy_ops mixel_dphy_phy_ops = {
> >  	.init = mixel_dphy_init,
> >  	.exit = mixel_dphy_exit,
> >  	.power_on = mixel_dphy_power_on,
> >  	.power_off = mixel_dphy_power_off,
> > +	.set_mode = mixel_dphy_set_mode,
> >  	.configure = mixel_dphy_configure,
> >  	.validate = mixel_dphy_validate,
> >  	.owner = THIS_MODULE,
> > @@ -424,6 +641,8 @@ static const struct phy_ops mixel_dphy_phy_ops = {
> >  static const struct of_device_id mixel_dphy_of_match[] = {
> >  	{ .compatible = "fsl,imx8mq-mipi-dphy",
> >  	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> > +	{ .compatible = "fsl,imx8qxp-mipi-dphy",
> > +	  .data = &mixel_dphy_devdata[MIXEL_IMX8QXP] },
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> > @@ -436,6 +655,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> >  	struct mixel_dphy_priv *priv;
> >  	struct phy *phy;
> >  	void __iomem *base;
> > +	int ret;
> >  
> >  	if (!np)
> >  		return -ENODEV;
> > @@ -467,6 +687,30 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> >  	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> >  		clk_get_rate(priv->phy_ref_clk));
> >  
> > +	if (priv->devdata->is_combo) {
> > +		priv->lvds_regmap =
> > +			syscon_regmap_lookup_by_phandle(np, "fsl,syscon");
> > +		if (IS_ERR(priv->lvds_regmap)) {
> > +			ret = PTR_ERR(priv->lvds_regmap);
> > +			dev_err_probe(dev, ret, "Failed to get LVDS regmap\n");
> > +			return ret;
> > +		}
> > +
> > +		priv->id = of_alias_get_id(np, "mipi_dphy");
> > +		if (priv->id < 0) {
> > +			dev_err(dev, "Failed to get phy node alias id: %d\n",
> > +								 priv->id);
> > +			return priv->id;
> > +		}
> > +
> > +		ret = imx_scu_get_handle(&priv->ipc_handle);
> > +		if (ret) {
> > +			dev_err_probe(dev, ret,
> > +				      "Failed to get SCU ipc handle\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	dev_set_drvdata(dev, priv);
> >  
> >  	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> > -- 
> > 2.7.4
> > 
> 
> Cheers,
>  -- Guido

Re: [PATCH 4/4] phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Guido Günther]

Hi,
On Tue, Dec 08, 2020 at 06:03:05PM +0800, Liu Ying wrote:
> On Tue, 2020-12-08 at 10:24 +0100, Guido Günther wrote:
> > Hi Liu,
> > some minor comments inline:
> > 
> > On Fri, Dec 04, 2020 at 03:33:44PM +0800, Liu Ying wrote:
> > > i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which supports
> > > either a MIPI DSI display or a LVDS display.  The PHY mode is controlled
> > > by SCU firmware and the driver would call a SCU firmware function to
> > > configure the PHY mode.  The single LVDS PHY has 4 data lanes to support
> > > a LVDS display.  Also, with a master LVDS PHY and a slave LVDS PHY, they
> > > may work together to support a LVDS display with 8 data lanes(usually, dual
> > > LVDS link display).  Note that this patch supports the LVDS PHY mode only
> > > for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet to be
> > > supported, so for now error would be returned from ->set_mode() if MIPI
> > > DPHY mode is passed over to it for the combo PHY.
> > > 
> > > Cc: Guido Günther <agx@sigxcpu.org>
> > > Cc: Robert Chiras <robert.chiras@nxp.com>
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > ---
> > >  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 266 ++++++++++++++++++++++++-
> > >  1 file changed, 255 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > index a95572b..37084a9 100644
> > > --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > @@ -4,17 +4,31 @@
> > >   * Copyright 2019 Purism SPC
> > >   */
> > >  
> > > +#include <linux/bitfield.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/clk-provider.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/firmware/imx/ipc.h>
> > > +#include <linux/firmware/imx/svc/misc.h>
> > >  #include <linux/io.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/mfd/syscon.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/phy/phy.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regmap.h>
> > > +#include <dt-bindings/firmware/imx/rsrc.h>
> > > +
> > > +/* Control and Status Registers(CSR) */
> > > +#define PHY_CTRL			0x00
> > > +#define  CCM_MASK			GENMASK(7, 5)
> > > +#define  CCM(n)				FIELD_PREP(CCM_MASK, (n))
> > > +#define  CA_MASK			GENMASK(4, 2)
> > > +#define  CA(n)				FIELD_PREP(CA_MASK, (n))
> > > +#define  RFB				BIT(1)
> > > +#define  LVDS_EN			BIT(0)
> > >  
> > >  /* DPHY registers */
> > >  #define DPHY_PD_DPHY			0x00
> > > @@ -55,8 +69,15 @@
> > >  #define PWR_ON	0
> > >  #define PWR_OFF	1
> > >  
> > > +#define MIN_VCO_FREQ 640000000
> > > +#define MAX_VCO_FREQ 1500000000
> > > +
> > > +#define MIN_LVDS_REFCLK_FREQ 24000000
> > > +#define MAX_LVDS_REFCLK_FREQ 150000000
> > > +
> > >  enum mixel_dphy_devtype {
> > >  	MIXEL_IMX8MQ,
> > > +	MIXEL_IMX8QXP,
> > >  };
> > >  
> > >  struct mixel_dphy_devdata {
> > > @@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
> > >  	u8 reg_rxlprp;
> > >  	u8 reg_rxcdrp;
> > >  	u8 reg_rxhs_settle;
> > > +	bool is_combo;	/* MIPI DPHY and LVDS PHY combo */
> > >  };
> > >  
> > >  static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > > @@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > >  		.reg_rxlprp = 0x40,
> > >  		.reg_rxcdrp = 0x44,
> > >  		.reg_rxhs_settle = 0x48,
> > > +		.is_combo = false,
> > > +	},
> > > +	[MIXEL_IMX8QXP] = {
> > > +		.is_combo = true,
> > >  	},
> > >  };
> > >  
> > > @@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
> > >  struct mixel_dphy_priv {
> > >  	struct mixel_dphy_cfg cfg;
> > >  	struct regmap *regmap;
> > > +	struct regmap *lvds_regmap;
> > >  	struct clk *phy_ref_clk;
> > >  	const struct mixel_dphy_devdata *devdata;
> > > +	struct imx_sc_ipc *ipc_handle;
> > > +	bool is_slave;
> > > +	int id;
> > >  };
> > >  
> > >  static const struct regmap_config mixel_dphy_regmap_config = {
> > > @@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy *phy)
> > >  	return 0;
> > >  }
> > >  
> > > -static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > > +static int
> > > +mixel_dphy_configure_mipi_dphy(struct phy *phy, union phy_configure_opts *opts)
> > >  {
> > >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > >  	struct mixel_dphy_cfg cfg = { 0 };
> > > @@ -345,15 +376,118 @@ static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > >  	return 0;
> > >  }
> > >  
> > > +static int
> > > +mixel_dphy_configure_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
> > > +	unsigned long data_rate;
> > > +	unsigned long fvco;
> > > +	u32 rsc;
> > > +	u32 co;
> > > +	int ret;
> > > +
> > > +	priv->is_slave = lvds_opts->is_slave;
> > > +
> > > +	/* LVDS interface pins */
> > > +	regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) | RFB);
> > > +
> > > +	/* enable MODE8 only for slave LVDS PHY */
> > > +	rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > > +	ret = imx_sc_misc_set_control(priv->ipc_handle, rsc, IMX_SC_C_DUAL_MODE,
> > > +				      lvds_opts->is_slave);
> > > +	if (ret) {
> > > +		dev_err(&phy->dev, "Failed to configure MODE8: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Choose an appropriate divider ratio to meet the requirement of
> > > +	 * PLL VCO frequency range.
> > > +	 *
> > > +	 *  -----  640MHz ~ 1500MHz   ------------      ---------------
> > > +	 * | VCO | ----------------> | CO divider | -> | LVDS data rate|
> > > +	 *  -----       FVCO          ------------      ---------------
> > > +	 *                            1/2/4/8 div     7 * differential_clk_rate
> > > +	 */
> > > +	data_rate = 7 * lvds_opts->differential_clk_rate;
> > > +	for (co = 1; co <= 8; co *= 2) {
> > > +		fvco = data_rate * co;
> > > +
> > > +		if (fvco >= MIN_VCO_FREQ)
> > > +			break;
> > > +	}
> > > +
> > > +	if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
> > > +		dev_err(&phy->dev, "VCO frequency %lu is out of range\n", fvco);
> > > +		return -ERANGE;
> > > +	}
> > > +
> > > +	/*
> > > +	 * CO is configurable, while CN and CM are not,
> > > +	 * as fixed ratios 1 and 7 are applied respectively.
> > > +	 */
> > > +	phy_write(phy, __ffs(co), DPHY_CO);
> > > +
> > > +	/* set reference clock rate */
> > > +	clk_set_rate(priv->phy_ref_clk, lvds_opts->differential_clk_rate);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > > +{
> > > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
> > > +		return mixel_dphy_configure_mipi_dphy(phy, opts);
> > > +	else if (phy->attrs.mode == PHY_MODE_LVDS)
> > > +		return mixel_dphy_configure_lvds_phy(phy, opts);
> > > +
> > > +	dev_err(&phy->dev, "Failed to configure PHY with invalid PHY mode\n");
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int
> > > +mixel_dphy_validate_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > > +{
> > > +	struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
> > > +
> > > +	if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
> > > +		dev_err(&phy->dev, "Invalid bits per LVDS data lane: %u\n",
> > > +					lvds_cfg->bits_per_lane_and_dclk_cycle);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (lvds_cfg->lanes != 4) {
> > > +		dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
> > > +						lvds_cfg->lanes);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
> > > +	    lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
> > > +		dev_err(&phy->dev,
> > > +			"Invalid LVDS differential clock rate: %lu\n",
> > > +					lvds_cfg->differential_clk_rate);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
> > >  			       union phy_configure_opts *opts)
> > >  {
> > > -	struct mixel_dphy_cfg cfg = { 0 };
> > > +	if (mode == PHY_MODE_MIPI_DPHY) {
> > > +		struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
> > >  
> > > -	if (mode != PHY_MODE_MIPI_DPHY)
> > > -		return -EINVAL;
> > > +		return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > > +							&mipi_dphy_cfg);
> > > +	} else if (mode == PHY_MODE_LVDS) {
> > > +		return mixel_dphy_validate_lvds_phy(phy, opts);
> > > +	}
> > >  
> > > -	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > > +	dev_err(&phy->dev, "Failed to validate PHY with invalid PHY mode\n");
> > 
> > Can you print the `mode` here so it becomes obvious from the error what
> > incorrect mode got passed in?
> 
> Will do in the next version.
> 
> > 
> > > +	return -EINVAL;
> > >  }
> > >  
> > >  static int mixel_dphy_init(struct phy *phy)
> > > @@ -373,27 +507,74 @@ static int mixel_dphy_exit(struct phy *phy)
> > >  	return 0;
> > >  }
> > >  
> > > -static int mixel_dphy_power_on(struct phy *phy)
> > > +static int mixel_dphy_power_on_mipi_dphy(struct phy *phy)
> > >  {
> > >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > >  	u32 locked;
> > >  	int ret;
> > >  
> > > -	ret = clk_prepare_enable(priv->phy_ref_clk);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > >  	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > >  	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> > >  				       locked, PLL_LOCK_SLEEP,
> > >  				       PLL_LOCK_TIMEOUT);
> > >  	if (ret < 0) {
> > >  		dev_err(&phy->dev, "Could not get DPHY lock (%d)!\n", ret);
> > > -		goto clock_disable;
> > > +		return ret;
> > >  	}
> > >  	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > >  
> > >  	return 0;
> > > +}
> > > +
> > > +static int mixel_dphy_power_on_lvds_phy(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	u32 locked;
> > > +	int ret;
> > > +
> > > +	regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, LVDS_EN);
> > > +
> > > +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > > +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > > +
> > > +	/* do not wait for slave LVDS PHY being locked */
> > > +	if (priv->is_slave)
> > > +		return 0;
> > > +
> > > +	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> > > +				       locked, PLL_LOCK_SLEEP,
> > > +				       PLL_LOCK_TIMEOUT);
> > > +	if (ret < 0) {
> > > +		dev_err(&phy->dev, "Could not get LVDS PHY lock (%d)!\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mixel_dphy_power_on(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(priv->phy_ref_clk);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY) {
> > > +		ret = mixel_dphy_power_on_mipi_dphy(phy);
> > > +	} else if (phy->attrs.mode == PHY_MODE_LVDS) {
> > > +		ret = mixel_dphy_power_on_lvds_phy(phy);
> > > +	} else {
> > > +		dev_err(&phy->dev,
> > > +			"Failed to power on PHY with invalid PHY mode\n");
> > 
> > Can you print the `mode` here so it becomes obvious from the error what
> > incorrect mode got passed in?
> 
> Will do in the next version.
> 
> > 
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	if (ret)
> > > +		goto clock_disable;
> > > +
> > > +	return 0;
> > >  clock_disable:
> > >  	clk_disable_unprepare(priv->phy_ref_clk);
> > >  	return ret;
> > > @@ -406,16 +587,52 @@ static int mixel_dphy_power_off(struct phy *phy)
> > >  	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> > >  	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> > >  
> > > +	if (phy->attrs.mode == PHY_MODE_LVDS)
> > > +		regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, 0);
> > > +
> > >  	clk_disable_unprepare(priv->phy_ref_clk);
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > +static int mixel_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	int ret;
> > > +
> > 
> > I'd reject all modes except PHY_MODE_MIPI_DPHY and PHY_MODE_MIPI_LVDS upfront...
> 
> It seems that this overall rejection would cause _double_ check
> together with the snippet right below, which should be avoided.
> 
> > 
> > > +	/* Currently, MIPI DPHY mode only, if it's not a combo PHY. */
> > > +	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
> > > +		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
> > > +		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
> > > +		return -EINVAL;
> > > +	}
> > 
> > ...and then just reject on whetehr `is_combo` is set or not. This makes
> > it easier to add more later.
> 
> The above snippet is rejecting on whether 'is_combo' is set or not.
> So, in short, the current implementation looks ok to me.
> Or, things still can be improved here?

I agree, i think what tripped me up is that the double negative check
comes first comined with the comment so i'd just drop the comment and
start with the positive one:

	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
		return -EINVAL;
	}

	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
		return -EINVAL;
	}

Cheers,
 -- Guido

> 
> Thanks,
> Liu Ying
> 
> > 
> > > +	if (priv->devdata->is_combo) {
> > > +		u32 rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > > +
> > > +		ret = imx_sc_misc_set_control(priv->ipc_handle,
> > > +					      rsc, IMX_SC_C_MODE,
> > > +					      mode == PHY_MODE_LVDS);
> > > +		if (ret) {
> > > +			dev_err(&phy->dev,
> > > +				"Failed to set PHY mode via SCU ipc: %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct phy_ops mixel_dphy_phy_ops = {
> > >  	.init = mixel_dphy_init,
> > >  	.exit = mixel_dphy_exit,
> > >  	.power_on = mixel_dphy_power_on,
> > >  	.power_off = mixel_dphy_power_off,
> > > +	.set_mode = mixel_dphy_set_mode,
> > >  	.configure = mixel_dphy_configure,
> > >  	.validate = mixel_dphy_validate,
> > >  	.owner = THIS_MODULE,
> > > @@ -424,6 +641,8 @@ static const struct phy_ops mixel_dphy_phy_ops = {
> > >  static const struct of_device_id mixel_dphy_of_match[] = {
> > >  	{ .compatible = "fsl,imx8mq-mipi-dphy",
> > >  	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> > > +	{ .compatible = "fsl,imx8qxp-mipi-dphy",
> > > +	  .data = &mixel_dphy_devdata[MIXEL_IMX8QXP] },
> > >  	{ /* sentinel */ },
> > >  };
> > >  MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> > > @@ -436,6 +655,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> > >  	struct mixel_dphy_priv *priv;
> > >  	struct phy *phy;
> > >  	void __iomem *base;
> > > +	int ret;
> > >  
> > >  	if (!np)
> > >  		return -ENODEV;
> > > @@ -467,6 +687,30 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> > >  	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> > >  		clk_get_rate(priv->phy_ref_clk));
> > >  
> > > +	if (priv->devdata->is_combo) {
> > > +		priv->lvds_regmap =
> > > +			syscon_regmap_lookup_by_phandle(np, "fsl,syscon");
> > > +		if (IS_ERR(priv->lvds_regmap)) {
> > > +			ret = PTR_ERR(priv->lvds_regmap);
> > > +			dev_err_probe(dev, ret, "Failed to get LVDS regmap\n");
> > > +			return ret;
> > > +		}
> > > +
> > > +		priv->id = of_alias_get_id(np, "mipi_dphy");
> > > +		if (priv->id < 0) {
> > > +			dev_err(dev, "Failed to get phy node alias id: %d\n",
> > > +								 priv->id);
> > > +			return priv->id;
> > > +		}
> > > +
> > > +		ret = imx_scu_get_handle(&priv->ipc_handle);
> > > +		if (ret) {
> > > +			dev_err_probe(dev, ret,
> > > +				      "Failed to get SCU ipc handle\n");
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >  	dev_set_drvdata(dev, priv);
> > >  
> > >  	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> > > -- 
> > > 2.7.4
> > > 
> > 
> > Cheers,
> >  -- Guido
> 

Re: [PATCH 4/4] phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Liu Ying]

Hi Guido,

On Thu, 2020-12-10 at 08:14 +0100, Guido Günther wrote:
> Hi,
> On Tue, Dec 08, 2020 at 06:03:05PM +0800, Liu Ying wrote:
> > On Tue, 2020-12-08 at 10:24 +0100, Guido Günther wrote:
> > > Hi Liu,
> > > some minor comments inline:
> > > 
> > > On Fri, Dec 04, 2020 at 03:33:44PM +0800, Liu Ying wrote:
> > > > i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which supports
> > > > either a MIPI DSI display or a LVDS display.  The PHY mode is controlled
> > > > by SCU firmware and the driver would call a SCU firmware function to
> > > > configure the PHY mode.  The single LVDS PHY has 4 data lanes to support
> > > > a LVDS display.  Also, with a master LVDS PHY and a slave LVDS PHY, they
> > > > may work together to support a LVDS display with 8 data lanes(usually, dual
> > > > LVDS link display).  Note that this patch supports the LVDS PHY mode only
> > > > for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet to be
> > > > supported, so for now error would be returned from ->set_mode() if MIPI
> > > > DPHY mode is passed over to it for the combo PHY.
> > > > 
> > > > Cc: Guido Günther <agx@sigxcpu.org>
> > > > Cc: Robert Chiras <robert.chiras@nxp.com>
> > > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > ---
> > > >  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 266 ++++++++++++++++++++++++-
> > > >  1 file changed, 255 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > > index a95572b..37084a9 100644
> > > > --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > > @@ -4,17 +4,31 @@
> > > >   * Copyright 2019 Purism SPC
> > > >   */
> > > >  
> > > > +#include <linux/bitfield.h>
> > > >  #include <linux/clk.h>
> > > >  #include <linux/clk-provider.h>
> > > >  #include <linux/delay.h>
> > > > +#include <linux/firmware/imx/ipc.h>
> > > > +#include <linux/firmware/imx/svc/misc.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/kernel.h>
> > > > +#include <linux/mfd/syscon.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/of_platform.h>
> > > >  #include <linux/phy/phy.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/regmap.h>
> > > > +#include <dt-bindings/firmware/imx/rsrc.h>
> > > > +
> > > > +/* Control and Status Registers(CSR) */
> > > > +#define PHY_CTRL			0x00
> > > > +#define  CCM_MASK			GENMASK(7, 5)
> > > > +#define  CCM(n)				FIELD_PREP(CCM_MASK, (n))
> > > > +#define  CA_MASK			GENMASK(4, 2)
> > > > +#define  CA(n)				FIELD_PREP(CA_MASK, (n))
> > > > +#define  RFB				BIT(1)
> > > > +#define  LVDS_EN			BIT(0)
> > > >  
> > > >  /* DPHY registers */
> > > >  #define DPHY_PD_DPHY			0x00
> > > > @@ -55,8 +69,15 @@
> > > >  #define PWR_ON	0
> > > >  #define PWR_OFF	1
> > > >  
> > > > +#define MIN_VCO_FREQ 640000000
> > > > +#define MAX_VCO_FREQ 1500000000
> > > > +
> > > > +#define MIN_LVDS_REFCLK_FREQ 24000000
> > > > +#define MAX_LVDS_REFCLK_FREQ 150000000
> > > > +
> > > >  enum mixel_dphy_devtype {
> > > >  	MIXEL_IMX8MQ,
> > > > +	MIXEL_IMX8QXP,
> > > >  };
> > > >  
> > > >  struct mixel_dphy_devdata {
> > > > @@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
> > > >  	u8 reg_rxlprp;
> > > >  	u8 reg_rxcdrp;
> > > >  	u8 reg_rxhs_settle;
> > > > +	bool is_combo;	/* MIPI DPHY and LVDS PHY combo */
> > > >  };
> > > >  
> > > >  static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > > > @@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > > >  		.reg_rxlprp = 0x40,
> > > >  		.reg_rxcdrp = 0x44,
> > > >  		.reg_rxhs_settle = 0x48,
> > > > +		.is_combo = false,
> > > > +	},
> > > > +	[MIXEL_IMX8QXP] = {
> > > > +		.is_combo = true,
> > > >  	},
> > > >  };
> > > >  
> > > > @@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
> > > >  struct mixel_dphy_priv {
> > > >  	struct mixel_dphy_cfg cfg;
> > > >  	struct regmap *regmap;
> > > > +	struct regmap *lvds_regmap;
> > > >  	struct clk *phy_ref_clk;
> > > >  	const struct mixel_dphy_devdata *devdata;
> > > > +	struct imx_sc_ipc *ipc_handle;
> > > > +	bool is_slave;
> > > > +	int id;
> > > >  };
> > > >  
> > > >  static const struct regmap_config mixel_dphy_regmap_config = {
> > > > @@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy *phy)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > > > +static int
> > > > +mixel_dphy_configure_mipi_dphy(struct phy *phy, union phy_configure_opts *opts)
> > > >  {
> > > >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > >  	struct mixel_dphy_cfg cfg = { 0 };
> > > > @@ -345,15 +376,118 @@ static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int
> > > > +mixel_dphy_configure_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > > > +{
> > > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > > +	struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
> > > > +	unsigned long data_rate;
> > > > +	unsigned long fvco;
> > > > +	u32 rsc;
> > > > +	u32 co;
> > > > +	int ret;
> > > > +
> > > > +	priv->is_slave = lvds_opts->is_slave;
> > > > +
> > > > +	/* LVDS interface pins */
> > > > +	regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) | RFB);
> > > > +
> > > > +	/* enable MODE8 only for slave LVDS PHY */
> > > > +	rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > > > +	ret = imx_sc_misc_set_control(priv->ipc_handle, rsc, IMX_SC_C_DUAL_MODE,
> > > > +				      lvds_opts->is_slave);
> > > > +	if (ret) {
> > > > +		dev_err(&phy->dev, "Failed to configure MODE8: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Choose an appropriate divider ratio to meet the requirement of
> > > > +	 * PLL VCO frequency range.
> > > > +	 *
> > > > +	 *  -----  640MHz ~ 1500MHz   ------------      ---------------
> > > > +	 * | VCO | ----------------> | CO divider | -> | LVDS data rate|
> > > > +	 *  -----       FVCO          ------------      ---------------
> > > > +	 *                            1/2/4/8 div     7 * differential_clk_rate
> > > > +	 */
> > > > +	data_rate = 7 * lvds_opts->differential_clk_rate;
> > > > +	for (co = 1; co <= 8; co *= 2) {
> > > > +		fvco = data_rate * co;
> > > > +
> > > > +		if (fvco >= MIN_VCO_FREQ)
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
> > > > +		dev_err(&phy->dev, "VCO frequency %lu is out of range\n", fvco);
> > > > +		return -ERANGE;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * CO is configurable, while CN and CM are not,
> > > > +	 * as fixed ratios 1 and 7 are applied respectively.
> > > > +	 */
> > > > +	phy_write(phy, __ffs(co), DPHY_CO);
> > > > +
> > > > +	/* set reference clock rate */
> > > > +	clk_set_rate(priv->phy_ref_clk, lvds_opts->differential_clk_rate);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > > > +{
> > > > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
> > > > +		return mixel_dphy_configure_mipi_dphy(phy, opts);
> > > > +	else if (phy->attrs.mode == PHY_MODE_LVDS)
> > > > +		return mixel_dphy_configure_lvds_phy(phy, opts);
> > > > +
> > > > +	dev_err(&phy->dev, "Failed to configure PHY with invalid PHY mode\n");
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +static int
> > > > +mixel_dphy_validate_lvds_phy(struct phy *phy, union phy_configure_opts *opts)
> > > > +{
> > > > +	struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
> > > > +
> > > > +	if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
> > > > +		dev_err(&phy->dev, "Invalid bits per LVDS data lane: %u\n",
> > > > +					lvds_cfg->bits_per_lane_and_dclk_cycle);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (lvds_cfg->lanes != 4) {
> > > > +		dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
> > > > +						lvds_cfg->lanes);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
> > > > +	    lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
> > > > +		dev_err(&phy->dev,
> > > > +			"Invalid LVDS differential clock rate: %lu\n",
> > > > +					lvds_cfg->differential_clk_rate);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
> > > >  			       union phy_configure_opts *opts)
> > > >  {
> > > > -	struct mixel_dphy_cfg cfg = { 0 };
> > > > +	if (mode == PHY_MODE_MIPI_DPHY) {
> > > > +		struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
> > > >  
> > > > -	if (mode != PHY_MODE_MIPI_DPHY)
> > > > -		return -EINVAL;
> > > > +		return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > > > +							&mipi_dphy_cfg);
> > > > +	} else if (mode == PHY_MODE_LVDS) {
> > > > +		return mixel_dphy_validate_lvds_phy(phy, opts);
> > > > +	}
> > > >  
> > > > -	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > > > +	dev_err(&phy->dev, "Failed to validate PHY with invalid PHY mode\n");
> > > 
> > > Can you print the `mode` here so it becomes obvious from the error what
> > > incorrect mode got passed in?
> > 
> > Will do in the next version.
> > 
> > > > +	return -EINVAL;
> > > >  }
> > > >  
> > > >  static int mixel_dphy_init(struct phy *phy)
> > > > @@ -373,27 +507,74 @@ static int mixel_dphy_exit(struct phy *phy)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int mixel_dphy_power_on(struct phy *phy)
> > > > +static int mixel_dphy_power_on_mipi_dphy(struct phy *phy)
> > > >  {
> > > >  	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > >  	u32 locked;
> > > >  	int ret;
> > > >  
> > > > -	ret = clk_prepare_enable(priv->phy_ref_clk);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > -
> > > >  	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > > >  	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> > > >  				       locked, PLL_LOCK_SLEEP,
> > > >  				       PLL_LOCK_TIMEOUT);
> > > >  	if (ret < 0) {
> > > >  		dev_err(&phy->dev, "Could not get DPHY lock (%d)!\n", ret);
> > > > -		goto clock_disable;
> > > > +		return ret;
> > > >  	}
> > > >  	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > > >  
> > > >  	return 0;
> > > > +}
> > > > +
> > > > +static int mixel_dphy_power_on_lvds_phy(struct phy *phy)
> > > > +{
> > > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > > +	u32 locked;
> > > > +	int ret;
> > > > +
> > > > +	regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, LVDS_EN);
> > > > +
> > > > +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > > > +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > > > +
> > > > +	/* do not wait for slave LVDS PHY being locked */
> > > > +	if (priv->is_slave)
> > > > +		return 0;
> > > > +
> > > > +	ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
> > > > +				       locked, PLL_LOCK_SLEEP,
> > > > +				       PLL_LOCK_TIMEOUT);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&phy->dev, "Could not get LVDS PHY lock (%d)!\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int mixel_dphy_power_on(struct phy *phy)
> > > > +{
> > > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_prepare_enable(priv->phy_ref_clk);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	if (phy->attrs.mode == PHY_MODE_MIPI_DPHY) {
> > > > +		ret = mixel_dphy_power_on_mipi_dphy(phy);
> > > > +	} else if (phy->attrs.mode == PHY_MODE_LVDS) {
> > > > +		ret = mixel_dphy_power_on_lvds_phy(phy);
> > > > +	} else {
> > > > +		dev_err(&phy->dev,
> > > > +			"Failed to power on PHY with invalid PHY mode\n");
> > > 
> > > Can you print the `mode` here so it becomes obvious from the error what
> > > incorrect mode got passed in?
> > 
> > Will do in the next version.
> > 
> > > > +		ret = -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (ret)
> > > > +		goto clock_disable;
> > > > +
> > > > +	return 0;
> > > >  clock_disable:
> > > >  	clk_disable_unprepare(priv->phy_ref_clk);
> > > >  	return ret;
> > > > @@ -406,16 +587,52 @@ static int mixel_dphy_power_off(struct phy *phy)
> > > >  	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> > > >  	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> > > >  
> > > > +	if (phy->attrs.mode == PHY_MODE_LVDS)
> > > > +		regmap_update_bits(priv->lvds_regmap, PHY_CTRL, LVDS_EN, 0);
> > > > +
> > > >  	clk_disable_unprepare(priv->phy_ref_clk);
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int mixel_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > > > +{
> > > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > > +	int ret;
> > > > +
> > > 
> > > I'd reject all modes except PHY_MODE_MIPI_DPHY and PHY_MODE_MIPI_LVDS upfront...
> > 
> > It seems that this overall rejection would cause _double_ check
> > together with the snippet right below, which should be avoided.
> > 
> > > > +	/* Currently, MIPI DPHY mode only, if it's not a combo PHY. */
> > > > +	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
> > > > +		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
> > > > +		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > ...and then just reject on whetehr `is_combo` is set or not. This makes
> > > it easier to add more later.
> > 
> > The above snippet is rejecting on whether 'is_combo' is set or not.
> > So, in short, the current implementation looks ok to me.
> > Or, things still can be improved here?
> 
> I agree, i think what tripped me up is that the double negative check
> comes first comined with the comment so i'd just drop the comment and
> start with the positive one:
> 
> 	if (priv->devdata->is_combo && mode != PHY_MODE_LVDS) {
> 		dev_err(&phy->dev, "Failed to set PHY mode for combo PHY\n");
> 		return -EINVAL;
> 	}
> 
> 	if (!priv->devdata->is_combo && mode != PHY_MODE_MIPI_DPHY) {
> 		dev_err(&phy->dev, "Failed to set PHY mode to MIPI DPHY\n");
> 		return -EINVAL;
> 	}

Ok.  I'll use this snippet.

Thanks,
Liu Ying

> 
> Cheers,
>  -- Guido
> 
> > Thanks,
> > Liu Ying
> > 
> > > > +	if (priv->devdata->is_combo) {
> > > > +		u32 rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > > > +
> > > > +		ret = imx_sc_misc_set_control(priv->ipc_handle,
> > > > +					      rsc, IMX_SC_C_MODE,
> > > > +					      mode == PHY_MODE_LVDS);
> > > > +		if (ret) {
> > > > +			dev_err(&phy->dev,
> > > > +				"Failed to set PHY mode via SCU ipc: %d\n", ret);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static const struct phy_ops mixel_dphy_phy_ops = {
> > > >  	.init = mixel_dphy_init,
> > > >  	.exit = mixel_dphy_exit,
> > > >  	.power_on = mixel_dphy_power_on,
> > > >  	.power_off = mixel_dphy_power_off,
> > > > +	.set_mode = mixel_dphy_set_mode,
> > > >  	.configure = mixel_dphy_configure,
> > > >  	.validate = mixel_dphy_validate,
> > > >  	.owner = THIS_MODULE,
> > > > @@ -424,6 +641,8 @@ static const struct phy_ops mixel_dphy_phy_ops = {
> > > >  static const struct of_device_id mixel_dphy_of_match[] = {
> > > >  	{ .compatible = "fsl,imx8mq-mipi-dphy",
> > > >  	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> > > > +	{ .compatible = "fsl,imx8qxp-mipi-dphy",
> > > > +	  .data = &mixel_dphy_devdata[MIXEL_IMX8QXP] },
> > > >  	{ /* sentinel */ },
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> > > > @@ -436,6 +655,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> > > >  	struct mixel_dphy_priv *priv;
> > > >  	struct phy *phy;
> > > >  	void __iomem *base;
> > > > +	int ret;
> > > >  
> > > >  	if (!np)
> > > >  		return -ENODEV;
> > > > @@ -467,6 +687,30 @@ static int mixel_dphy_probe(struct platform_device *pdev)
> > > >  	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> > > >  		clk_get_rate(priv->phy_ref_clk));
> > > >  
> > > > +	if (priv->devdata->is_combo) {
> > > > +		priv->lvds_regmap =
> > > > +			syscon_regmap_lookup_by_phandle(np, "fsl,syscon");
> > > > +		if (IS_ERR(priv->lvds_regmap)) {
> > > > +			ret = PTR_ERR(priv->lvds_regmap);
> > > > +			dev_err_probe(dev, ret, "Failed to get LVDS regmap\n");
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		priv->id = of_alias_get_id(np, "mipi_dphy");
> > > > +		if (priv->id < 0) {
> > > > +			dev_err(dev, "Failed to get phy node alias id: %d\n",
> > > > +								 priv->id);
> > > > +			return priv->id;
> > > > +		}
> > > > +
> > > > +		ret = imx_scu_get_handle(&priv->ipc_handle);
> > > > +		if (ret) {
> > > > +			dev_err_probe(dev, ret,
> > > > +				      "Failed to get SCU ipc handle\n");
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	dev_set_drvdata(dev, priv);
> > > >  
> > > >  	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > Cheers,
> > >  -- Guido

Re: [PATCH 0/4] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Guido Günther]

Hi Liu,
On Fri, Dec 04, 2020 at 03:33:40PM +0800, Liu Ying wrote:
> Hi,
> 
> This series adds i.MX8qxp LVDS PHY mode support for the Mixel PHY in the
> Freescale i.MX8qxp SoC.

This looks good to me from the NWL and actual phy driver part. I'll
comment in the individual patches but leave comments on the extension
of the generic phy struct to someone knowledgeable with that part.

What display controllers do you intend to drive that with?
Cheers,
 -- Guido

> 
> The Mixel PHY is MIPI DPHY + LVDS PHY combo, which can works in either
> MIPI DPHY mode or LVDS PHY mode.  The PHY mode is controlled by i.MX8qxp
> SCU firmware.  The PHY driver would call a SCU function to configure the
> mode.
> 
> The PHY driver is already supporting the Mixel MIPI DPHY in i.MX8mq SoC,
> where it appears to be a single MIPI DPHY.
> 
> 
> Patch 1/4 sets PHY mode in the Northwest Logic MIPI DSI host controller
> bridge driver, since i.MX8qxp SoC embeds this controller IP to support
> MIPI DSI displays together with the Mixel PHY.
> 
> Patch 2/4 allows LVDS PHYs to be configured through the generic PHY functions
> and through a custom structure added to the generic PHY configuration union.
> 
> Patch 3/4 adds dt binding support for the Mixel combo PHY in i.MX8qxp SoC.
> 
> Patch 4/4 adds the i.MX8qxp LVDS PHY mode support in the Mixel PHY driver.
> 
> 
> Welcome comments, thanks.
> 
> 
> Liu Ying (4):
>   drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()
>   phy: Add LVDS configuration options
>   dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for
>     i.MX8qxp
>   phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode
>     support
> 
>  .../devicetree/bindings/phy/mixel,mipi-dsi-phy.txt |   8 +-
>  drivers/gpu/drm/bridge/nwl-dsi.c                   |   6 +
>  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c     | 266 ++++++++++++++++++++-
>  include/linux/phy/phy-lvds.h                       |  48 ++++
>  include/linux/phy/phy.h                            |   4 +
>  5 files changed, 320 insertions(+), 12 deletions(-)
>  create mode 100644 include/linux/phy/phy-lvds.h
> 
> -- 
> 2.7.4
> 

Re: [PATCH 0/4] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

[Liu Ying]

Hi Guido,

On Tue, 2020-12-08 at 10:02 +0100, Guido Günther wrote:
> Hi Liu,
> On Fri, Dec 04, 2020 at 03:33:40PM +0800, Liu Ying wrote:
> > Hi,
> > 
> > This series adds i.MX8qxp LVDS PHY mode support for the Mixel PHY in the
> > Freescale i.MX8qxp SoC.
> 
> This looks good to me from the NWL and actual phy driver part. I'll
> comment in the individual patches but leave comments on the extension
> of the generic phy struct to someone knowledgeable with that part.

Thank you for the review.

> 
> What display controllers do you intend to drive that with?

The display controller DPU embedded in i.MX8qxp SoC would drive the
MIPI DSI display or the LVDS display through the Mixel combo PHY.

I've sent out a series to add DPU DRM driver support(KMS part only so
far) for review:
https://www.spinics.net/lists/kernel/msg3762462.html

I can Cc you when I send the next version for it.

Regards,
Liu Ying

> Cheers,
>  -- Guido
> 
> > The Mixel PHY is MIPI DPHY + LVDS PHY combo, which can works in either
> > MIPI DPHY mode or LVDS PHY mode.  The PHY mode is controlled by i.MX8qxp
> > SCU firmware.  The PHY driver would call a SCU function to configure the
> > mode.
> > 
> > The PHY driver is already supporting the Mixel MIPI DPHY in i.MX8mq SoC,
> > where it appears to be a single MIPI DPHY.
> > 
> > 
> > Patch 1/4 sets PHY mode in the Northwest Logic MIPI DSI host controller
> > bridge driver, since i.MX8qxp SoC embeds this controller IP to support
> > MIPI DSI displays together with the Mixel PHY.
> > 
> > Patch 2/4 allows LVDS PHYs to be configured through the generic PHY functions
> > and through a custom structure added to the generic PHY configuration union.
> > 
> > Patch 3/4 adds dt binding support for the Mixel combo PHY in i.MX8qxp SoC.
> > 
> > Patch 4/4 adds the i.MX8qxp LVDS PHY mode support in the Mixel PHY driver.
> > 
> > 
> > Welcome comments, thanks.
> > 
> > 
> > Liu Ying (4):
> >   drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()
> >   phy: Add LVDS configuration options
> >   dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for
> >     i.MX8qxp
> >   phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode
> >     support
> > 
> >  .../devicetree/bindings/phy/mixel,mipi-dsi-phy.txt |   8 +-
> >  drivers/gpu/drm/bridge/nwl-dsi.c                   |   6 +
> >  drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c     | 266 ++++++++++++++++++++-
> >  include/linux/phy/phy-lvds.h                       |  48 ++++
> >  include/linux/phy/phy.h                            |   4 +
> >  5 files changed, 320 insertions(+), 12 deletions(-)
> >  create mode 100644 include/linux/phy/phy-lvds.h
> > 
> > -- 
> > 2.7.4
> >