mbox series

[v4,0/5] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support

Message ID 1615175541-29009-1-git-send-email-victor.liu@nxp.com
Headers show
Series phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support | expand

Message

Liu Ying March 8, 2021, 3:52 a.m. UTC 
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/5 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/5 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/5 converts mixel,mipi-dsi-phy plain text dt binding to json-schema.

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

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


Welcome comments, thanks.

v3->v4:
* Add all R-b tags recieved from v3 on relevant patches and respin. (Robert)

v2->v3:
* Improve readability of mixel_dphy_set_mode() in the Mixel PHY driver. (Guido)
* Improve the 'clock-names' property in the PHY dt binding.

v1->v2:
* Convert mixel,mipi-dsi-phy plain text dt binding to json-schema. (Guido)
* Print invalid PHY mode in dmesg from the Mixel PHY driver. (Guido)
* Add Guido's R-b tag on the patch for the nwl-dsi drm bridge driver.

Liu Ying (5):
  drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable()
  phy: Add LVDS configuration options
  dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema
  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 |  29 ---
 .../bindings/phy/mixel,mipi-dsi-phy.yaml           | 107 ++++++++
 drivers/gpu/drm/bridge/nwl-dsi.c                   |   6 +
 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c     | 269 ++++++++++++++++++++-
 include/linux/phy/phy-lvds.h                       |  48 ++++
 include/linux/phy/phy.h                            |   4 +
 6 files changed, 423 insertions(+), 40 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
 create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml
 create mode 100644 include/linux/phy/phy-lvds.h

Comments

Vinod Koul March 17, 2021, 10:22 a.m. UTC | #1 
On 08-03-21, 11:52, 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>
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> v3->v4:
> * Add Robert's R-b tag.
> 
> v2->v3:
> * No change.
> 
> v1->v2:
> * No change.
> 
>  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.
> +	 */

Can we have these in kernel-doc style please, similar to style in linux/phy/phy.h

> +	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
Liu Ying March 18, 2021, 2:22 a.m. UTC | #2 
Hi Vinod,

On Wed, 2021-03-17 at 15:52 +0530, Vinod Koul wrote:
> On 08-03-21, 11:52, 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>
> > Reviewed-by: Robert Foss <robert.foss@linaro.org>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > v3->v4:
> > * Add Robert's R-b tag.
> > 
> > v2->v3:
> > * No change.
> > 
> > v1->v2:
> > * No change.
> > 
> >  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.
> > +	 */
> 
> Can we have these in kernel-doc style please, similar to style in linux/phy/phy.h

I take this way of in-line member documentation comment for the below 3
reasons:

1) Members of struct phy_configure_opts_mipi_dphy and
struct phy_configure_opts_dp use the same way of comment.
The structures are defined in linux/phy/phy-mipi-dphy.h and
linux/phy/phy-dp.h respectively.
Aligning to them makes a bit sense, IMHO.

2) In-line member documentation comments[1] are mentioned in kernel-doc 
guide. It says 'The structure members may also be documented in-line
within the definition.'.

3) Even the 'configure' and 'validate' members of struct phy_ops use
the same way of comment.  struct phy_ops is defined in linux/phy/phy.h.

[1] 
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments

Regards,
Liu Ying

> 
> > +	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
Vinod Koul March 25, 2021, 7:28 a.m. UTC | #3 
On 18-03-21, 10:22, Liu Ying wrote:

> > Can we have these in kernel-doc style please, similar to style in linux/phy/phy.h
> 
> I take this way of in-line member documentation comment for the below 3
> reasons:
> 
> 1) Members of struct phy_configure_opts_mipi_dphy and
> struct phy_configure_opts_dp use the same way of comment.
> The structures are defined in linux/phy/phy-mipi-dphy.h and
> linux/phy/phy-dp.h respectively.
> Aligning to them makes a bit sense, IMHO.
> 
> 2) In-line member documentation comments[1] are mentioned in kernel-doc 
> guide. It says 'The structure members may also be documented in-line
> within the definition.'.
> 
> 3) Even the 'configure' and 'validate' members of struct phy_ops use
> the same way of comment.  struct phy_ops is defined in linux/phy/phy.h.
> 
> [1] 
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments

It 'may be' but I would like all headers of a subsystem to display one
style. As I said linux/phy/phy.h use a style which we should use
everywhere.

Thanks
Liu Ying March 25, 2021, 9:30 a.m. UTC | #4 
Hi Vinod,

On Thu, 2021-03-25 at 12:58 +0530, Vinod Koul wrote:
> On 18-03-21, 10:22, Liu Ying wrote:
> 
> > > Can we have these in kernel-doc style please, similar to style in linux/phy/phy.h
> > 
> > I take this way of in-line member documentation comment for the below 3
> > reasons:
> > 
> > 1) Members of struct phy_configure_opts_mipi_dphy and
> > struct phy_configure_opts_dp use the same way of comment.
> > The structures are defined in linux/phy/phy-mipi-dphy.h and
> > linux/phy/phy-dp.h respectively.
> > Aligning to them makes a bit sense, IMHO.
> > 
> > 2) In-line member documentation comments[1] are mentioned in kernel-doc 
> > guide. It says 'The structure members may also be documented in-line
> > within the definition.'.
> > 
> > 3) Even the 'configure' and 'validate' members of struct phy_ops use
> > the same way of comment.  struct phy_ops is defined in linux/phy/phy.h.
> > 
> > [1] 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fdoc-guide%2Fkernel-doc.html%23in-line-member-documentation-comments&amp;data=04%7C01%7Cvictor.liu%40nxp.com%7C5f33165920d0484dec4d08d8ef5faabe%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637522541498852343%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=l25%2FXc0Xu2aH%2Fi7cSUqTsKae9L6BTddbhbV3vVRyON0%3D&amp;reserved=0
> 
> It 'may be' but I would like all headers of a subsystem to display one
> style. As I said linux/phy/phy.h use a style which we should use
> everywhere.

I've sent v5 out with this comment addressed.

Thanks,
Liu Ying

> 
> Thanks
>