[v2,3/3] phy: Add driver for mixel dphy found on imx8

Commit Message

Guido Günther Feb. 1, 2019, 8:49 a.m. UTC

This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
this is an IP core it will likely be found on others in the future. So
instead of adding this to the nwl host driver make it a generic PHY
driver.

The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be
added once the necessary system controller bits are in via
mixel_dphy_devdata.

Signed-off-by: Guido Günther <[e-mail]>
---
 drivers/phy/freescale/Kconfig                 |   9 +
 drivers/phy/freescale/Makefile                |   1 +
 .../phy/freescale/phy-fsl-imx8-mipi-dphy.c    | 494 ++++++++++++++++++
 3 files changed, 504 insertions(+)
 create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c

Comments

Fabio Estevam Feb. 1, 2019, 11:26 a.m. UTC | #1

Hi Guido,

Thanks for the respin. It looks better :-)

On Fri, Feb 1, 2019 at 6:50 AM Guido Günther <[e-mail]> wrote:

> +config PHY_MIXEL_MIPI_DPHY
> +       tristate "Mixel MIPI DSI PHY support"
> +       depends on OF
> +       select GENERIC_PHY
> +       select GENERIC_PHY_MIPI_DPHY

Since you converted to regmap, I guess you need:
select REGMAP_MMIO now?

> +       help
> +         Enable this to add support for the Mixel DSI PHY as found
> +         on NXP's i.MX8 family of SOCs.
> diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
> index dc2b3f1f2f80..07491c926a2c 100644
> --- a/drivers/phy/freescale/Makefile
> +++ b/drivers/phy/freescale/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       += phy-fsl-imx8mq-usb.o
> +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      += phy-fsl-imx8-mipi-dphy.o
> diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> new file mode 100644
> index 000000000000..4b182f2eaa6e
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> @@ -0,0 +1,494 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017,2018 NXP
> + * Copyright 2019 Purism SPC
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +
> +/* DPHY registers */
> +#define DPHY_PD_DPHY                   0x00
> +#define DPHY_M_PRG_HS_PREPARE          0x04
> +#define DPHY_MC_PRG_HS_PREPARE         0x08
> +#define DPHY_M_PRG_HS_ZERO             0x0c
> +#define DPHY_MC_PRG_HS_ZERO            0x10
> +#define DPHY_M_PRG_HS_TRAIL            0x14
> +#define DPHY_MC_PRG_HS_TRAIL           0x18
> +#define DPHY_PD_PLL                    0x1c
> +#define DPHY_TST                       0x20
> +#define DPHY_CN                                0x24
> +#define DPHY_CM                                0x28
> +#define DPHY_CO                                0x2c
> +#define DPHY_LOCK                      0x30
> +#define DPHY_LOCK_BYP                  0x34
> +#define DPHY_REG_BYPASS_PLL            0x4C
> +
> +#define MBPS(x) ((x) * 1000000)
> +
> +#define DATA_RATE_MAX_SPEED MBPS(1500)
> +#define DATA_RATE_MIN_SPEED MBPS(80)
> +
> +#define CN_BUF 0xcb7a89c0
> +#define CO_BUF 0x63
> +#define CM(x)  (                               \
> +               ((x) <  32)?0xe0|((x)-16) :     \

Doesn't checkpatch complain about the need of space between operators?

> +               ((x) <  64)?0xc0|((x)-32) :     \
> +               ((x) < 128)?0x80|((x)-64) :     \
> +               ((x) - 128))
> +#define CN(x)  (((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
> +#define CO(x)  ((CO_BUF)>>(8-(x))&0x3)
> +
> +/* PHY power on is LOW_ENABLE */

active low is probably a better term.

> +#define PWR_ON 0
> +#define PWR_OFF        1

> +static inline u32 phy_read(struct phy *phy, unsigned int reg)

After the conversion to regmap this function is unused now and you
should remove it.

> +static inline void phy_write(struct phy *phy, u32 value, unsigned int reg)

No need for "inline".

Make it static int instead.

> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +       int ret;
> +
> +       ret = regmap_write(priv->regs, reg, value);

Or maybe get rid of this function and use regmap_write() instead.

> +       frequency = ref_clk * numerator / (2 * denominator);
> +       dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",

dev_dbg() would be better?

> +                frequency, dphy_opts->hs_clk_rate, ref_clk,
> +                numerator, denominator);
> +
> +       /* LP clock period */
> +       lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> +       dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> +               dphy_opts->lp_clk_rate, lp_t);
> +       /*
> +        *  hs_prepare: in lp clock periods
> +        */

Please use single line comment style instead.

> +       if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> +               dev_err(&phy->dev,
> +                       "hs_prepare (%u) > 2.5 * lp clock period (%lu)",
> +                       dphy_opts->hs_prepare, lp_t);
> +               return -EINVAL;
> +       }
> +       /* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
> +       if (dphy_opts->hs_prepare < lp_t)
> +               n = 0;
> +       else
> +               n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> +       cfg->m_prg_hs_prepare = n;
> +
> +       /*
> +        * clk_prepare: in lp clock periods
> +        */

Same here.

> +       if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> +               dev_err(&phy->dev,
> +                       "clk_prepare (%u) > 1.5 * lp clock period (%lu)",
> +                       dphy_opts->clk_prepare, lp_t);
> +               return -EINVAL;
> +       }
> +       /* 00: lp_t, 01: 1.5 * lp_t */
> +       cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 : 0;
> +
> +       /*
> +        * hs_zero: forumula from NXP BSP

Typo: formula

> +        */
> +       n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) / 10000;
> +       cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> +
> +       /*
> +        * clk_zero: forumula from NXP BSP

Typo: formula

> +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +       struct mixel_dphy_cfg cfg = { 0 };
> +       int ret;
> +
> +       ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> +       if (ret)
> +               return ret;
> +
> +       /* Update the configuration */
> +       memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> +
> +       dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",

You have already printed CM, CN, CO above. No need to printed again.

> +static int mixel_dphy_power_on(struct phy *phy)
> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +       u32 locked;
> +
> +       clk_prepare_enable(priv->phy_ref_clk);

clk_prepare_enable() may fail. Better do:

ret = clk_prepare_enable(priv->phy_ref_clk);
if (ret < 0)
    return ret;

> +
> +       phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> +       phy_write(phy, PWR_ON, DPHY_PD_PLL);
> +
> +       if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
> +                                    locked, 10, 1000) < 0) {
> +               dev_err(&phy->dev, "Could not get DPHY lock!\n");
> +               return -EINVAL;

Please add defines for 10 and 1000.

Better propagate the real error value here:

ret = regmap_read_poll_timeout(...)
if (ret) {
   dev_err();
   goto clock_disable
}

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;

You can remove the NULL check ...

> +
> +       regs = devm_ioremap(dev, res->start, resource_size(res));

If you use devm_ioremap_resource() instead.

kernel test robot Feb. 3, 2019, 9:32 a.m. UTC | #2

Hi Guido,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on next-20190201]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Guido-G-nther/Mixel-DPHY-support-for-i-MX8/20190202-015852
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
config: um-allyesconfig (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um 

All errors (new ones prefixed by >>):

   /usr/bin/ld: arch/um/drivers/vde.o: in function `vde_open_real':
   (.text+0x7d1): warning: Using 'getgrnam' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: (.text+0x61c): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: arch/um/drivers/vector_user.o: in function `user_init_socket_fds':
   vector_user.c:(.text+0x29d): warning: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: arch/um/drivers/pcap.o: in function `pcap_nametoaddr':
   (.text+0xde75): warning: Using 'gethostbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: arch/um/drivers/pcap.o: in function `pcap_nametonetaddr':
   (.text+0xdf15): warning: Using 'getnetbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: arch/um/drivers/pcap.o: in function `pcap_nametoproto':
   (.text+0xe135): warning: Using 'getprotobyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: arch/um/drivers/pcap.o: in function `pcap_nametoport':
   (.text+0xdf67): warning: Using 'getservbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   /usr/bin/ld: drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.o: in function `mixel_dphy_probe':
>> phy-fsl-imx8-mipi-dphy.c:(.text+0x3d0): undefined reference to `devm_ioremap'
>> /usr/bin/ld: phy-fsl-imx8-mipi-dphy.c:(.text+0x440): undefined reference to `__devm_regmap_init_mmio_clk'
   collect2: error: ld returned 1 exit status

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Robert Chiras Feb. 6, 2019, 3:28 p.m. UTC | #3

Hi Guido,

Thanks for picking this up. It's interesting to see that a lot has
changed in the PHY API and the phy can be now configured through the
API instead of exported function as I did in the NXP tree.

I was going through your implementation and I noticed you also added
the phy_ref clock to this driver too. This is good, since the DPHY
needs this clock, but I have a question related to the other clocks:
According to the Northwest Logic reference manual (the DSI host that
uses this DPHY), the host relies on the TX clock in order to configure
the DPHY. Is this driver relying on it's user to also enable the TX
clock?

Also: did you test this driver? Because I have a version of the patches
from NXP tree rebased on top of latest linux-next and I have a working
version of eLCDIF with Raydium RM67191 DSI panel on mScale850D
(i.MX8MQ). And I tried using this driver but there is no signal on the
screen, even through the register values are all identical. Next, I'll
try to debug why isn't this working on my setup.

Other than the above questions, I have some suggestions inline.

Best regards,
Robert


On Vi, 2019-02-01 at 09:49 +0100, Guido Günther wrote:
> This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
> this is an IP core it will likely be found on others in the future.
> So
> instead of adding this to the nwl host driver make it a generic PHY
> driver.
> 
> The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can
> be
> added once the necessary system controller bits are in via
> mixel_dphy_devdata.
> 
> Signed-off-by: Guido Günther <[e-mail]>
> ---
>  drivers/phy/freescale/Kconfig                 |   9 +
>  drivers/phy/freescale/Makefile                |   1 +
>  .../phy/freescale/phy-fsl-imx8-mipi-dphy.c    | 494
> ++++++++++++++++++
>  3 files changed, 504 insertions(+)
>  create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> 
> diff --git a/drivers/phy/freescale/Kconfig
> b/drivers/phy/freescale/Kconfig
> index 832670b4952b..43a5ca25d44b 100644
> --- a/drivers/phy/freescale/Kconfig
> +++ b/drivers/phy/freescale/Kconfig
> @@ -3,3 +3,12 @@ config PHY_FSL_IMX8MQ_USB
>  	depends on OF && HAS_IOMEM
>  	select GENERIC_PHY
>  	default ARCH_MXC && ARM64
> +
> +config PHY_MIXEL_MIPI_DPHY
> +	tristate "Mixel MIPI DSI PHY support"
> +	depends on OF
> +	select GENERIC_PHY
> +	select GENERIC_PHY_MIPI_DPHY
> +	help
> +	  Enable this to add support for the Mixel DSI PHY as found
> +	  on NXP's i.MX8 family of SOCs.
> diff --git a/drivers/phy/freescale/Makefile
> b/drivers/phy/freescale/Makefile
> index dc2b3f1f2f80..07491c926a2c 100644
> --- a/drivers/phy/freescale/Makefile
> +++ b/drivers/phy/freescale/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
> +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
> diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> new file mode 100644
> index 000000000000..4b182f2eaa6e
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> @@ -0,0 +1,494 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017,2018 NXP
> + * Copyright 2019 Purism SPC
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +
> +/* DPHY registers */
> +#define DPHY_PD_DPHY			0x00
> +#define DPHY_M_PRG_HS_PREPARE		0x04
> +#define DPHY_MC_PRG_HS_PREPARE		0x08
> +#define DPHY_M_PRG_HS_ZERO		0x0c
> +#define DPHY_MC_PRG_HS_ZERO		0x10
> +#define DPHY_M_PRG_HS_TRAIL		0x14
> +#define DPHY_MC_PRG_HS_TRAIL		0x18
> +#define DPHY_PD_PLL			0x1c
> +#define DPHY_TST			0x20
> +#define DPHY_CN				0x24
> +#define DPHY_CM				0x28
> +#define DPHY_CO				0x2c
> +#define DPHY_LOCK			0x30
> +#define DPHY_LOCK_BYP			0x34
> +#define DPHY_REG_BYPASS_PLL		0x4C
> +
> +#define MBPS(x) ((x) * 1000000)
> +
> +#define DATA_RATE_MAX_SPEED MBPS(1500)
> +#define DATA_RATE_MIN_SPEED MBPS(80)
> +
> +#define CN_BUF	0xcb7a89c0
> +#define CO_BUF	0x63
> +#define CM(x)	(				\
> +		((x) <	32)?0xe0|((x)-16) :	\
> +		((x) <	64)?0xc0|((x)-32) :	\
> +		((x) < 128)?0x80|((x)-64) :	\
> +		((x) - 128))
> +#define CN(x)	(((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
> +#define CO(x)	((CO_BUF)>>(8-(x))&0x3)
> +
> +/* PHY power on is LOW_ENABLE */
> +#define PWR_ON	0
> +#define PWR_OFF	1
> +
> +enum mixel_dphy_devtype {
> +	MIXEL_IMX8MQ,
> +};
> +
> +struct mixel_dphy_devdata {
> +	u8 reg_tx_rcal;
> +	u8 reg_auto_pd_en;
> +	u8 reg_rxlprp;
> +	u8 reg_rxcdrp;
> +	u8 reg_rxhs_settle;
> +};
> +
> +static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> +	[MIXEL_IMX8MQ] = {
> +		.reg_tx_rcal = 0x38,
> +		.reg_auto_pd_en = 0x3c,
> +		.reg_rxlprp = 0x40,
> +		.reg_rxcdrp = 0x44,
> +		.reg_rxhs_settle = 0x48,
> +	},
> +};
> +
> +struct mixel_dphy_cfg {
> +	/* DPHY PLL parameters */
> +	u32 cm;
> +	u32 cn;
> +	u32 co;
> +	/* DPHY register values */
> +	u8 mc_prg_hs_prepare;
> +	u8 m_prg_hs_prepare;
> +	u8 mc_prg_hs_zero;
> +	u8 m_prg_hs_zero;
> +	u8 mc_prg_hs_trail;
> +	u8 m_prg_hs_trail;
> +	u8 rxhs_settle;
> +};
> +
> +struct mixel_dphy_priv {
> +	struct mixel_dphy_cfg cfg;
> +	struct regmap *regs;
> +	struct clk *phy_ref_clk;
> +	const struct mixel_dphy_devdata *devdata;
> +};
> +
> +static const struct regmap_config mixel_dphy_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = DPHY_REG_BYPASS_PLL,
> +	.name = "mipi-dphy",
> +};
> +
> +static inline u32 phy_read(struct phy *phy, unsigned int reg)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(priv->regs, reg, &val);
> +	if (ret < 0)
> +		dev_err(&phy->dev, "Failed to read DPHY reg %d: %d",
> reg, ret);
> +	return val;
> +}
> +
> +static inline void phy_write(struct phy *phy, u32 value, unsigned
> int reg)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = regmap_write(priv->regs, reg, value);
> +	if (ret < 0)
> +		dev_err(&phy->dev, "Failed to write DPHY reg %d:
> %d", reg, ret);
> +}
> +
> +/*
> + * Find a ratio close to the desired one using continued fraction
> + * approximation ending either at exact match or maximum allowed
> + * nominator, denominator.
> + */
> +static void get_best_ratio(unsigned long *pnum, unsigned long
> *pdenom,
> +			   unsigned int max_n, unsigned int max_d)
> +{
> +	unsigned long a = *pnum;
> +	unsigned long b = *pdenom;
> +	unsigned long c;
> +	unsigned int n[] = {0, 1};
> +	unsigned int d[] = {1, 0};
> +	unsigned int whole;
> +	unsigned int i = 1;
> +
> +	while (b) {
> +		i ^= 1;
> +		whole = a / b;
> +		n[i] += (n[i ^ 1] * whole);
> +		d[i] += (d[i ^ 1] * whole);
> +		if ((n[i] > max_n) || (d[i] > max_d)) {
> +			i ^= 1;
> +			break;
> +		}
> +		c = a - (b * whole);
> +		a = b;
> +		b = c;
> +	}
> +	*pnum = n[i];
> +	*pdenom = d[i];
> +}
> +
> +static int mixel_dphy_config_from_opts(struct phy *phy,
> +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> +	       struct mixel_dphy_cfg *cfg)
> +{
> +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> >dev.parent);
> +	unsigned long ref_clk = clk_get_rate(priv->phy_ref_clk);
> +	unsigned long lp_t, numerator, denominator, frequency;
> +	u32 n;
> +	int i;
> +
> +	if (dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED ||
> +	    dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED)
> +		return -EINVAL;
> +
> +	numerator = dphy_opts->hs_clk_rate;
> +	denominator = ref_clk;
> +	get_best_ratio(&numerator, &denominator, 255, 256);
> +	if (!numerator || !denominator) {
> +		dev_err(&phy->dev, "Invalid %ld/%ld for %ld/%ld\n",
> +			numerator, denominator,
> +			dphy_opts->hs_clk_rate, ref_clk);
> +		return -EINVAL;
> +	}
> +
> +	while ((numerator < 16) && (denominator <= 128)) {
> +		numerator <<= 1;
> +		denominator <<= 1;
> +	}
> +	/*
> +	 * CM ranges between 16 and 255
> +	 * CN ranges between 1 and 32
> +	 * CO is power of 2: 1, 2, 4, 8
> +	 */
> +	i = __ffs(denominator);
> +	if (i > 3)
> +		i = 3;
> +	cfg->cn = denominator >> i;
> +	cfg->co = 1 << i;
> +	cfg->cm = numerator;
> +
> +	if (cfg->cm < 16 || cfg->cm > 255 ||
> +	    cfg->cn < 1 || cfg->cn > 32 ||
> +	    cfg->co < 1 || cfg->co > 8) {
> +		dev_err(&phy->dev, "Invalid CM/CN/CO values:
> %u/%u/%u\n",
> +			cfg->cm, cfg->cn, cfg->co);
> +		dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰
> %ld/%ld\n",
> +			dphy_opts->hs_clk_rate, ref_clk,
> +			numerator, denominator);
> +		return -EINVAL;
> +	}
> +
> +	frequency = ref_clk * numerator / (2 * denominator);
> +	dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰
> %ld/%ld\n",
> +		 frequency, dphy_opts->hs_clk_rate, ref_clk,
> +		 numerator, denominator);
> +
> +	/* LP clock period */
> +	lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> +	dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> +		dphy_opts->lp_clk_rate, lp_t);
> +	/*
> +	 *  hs_prepare: in lp clock periods
> +	 */
> +	if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> +		dev_err(&phy->dev,
> +			"hs_prepare (%u) > 2.5 * lp clock period
> (%lu)",
> +			dphy_opts->hs_prepare, lp_t);
> +		return -EINVAL;
> +	}
> +	/* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
> +	if (dphy_opts->hs_prepare < lp_t)
> +		n = 0;
> +	else
> +		n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> +	cfg->m_prg_hs_prepare = n;
> +
> +	/*
> +	 * clk_prepare: in lp clock periods
> +	 */
> +	if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> +		dev_err(&phy->dev,
> +			"clk_prepare (%u) > 1.5 * lp clock period
> (%lu)",
> +			dphy_opts->clk_prepare, lp_t);
> +		return -EINVAL;
> +	}
> +	/* 00: lp_t, 01: 1.5 * lp_t */
> +	cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 :
> 0;

Since hs_prepare and clk_prepare both depend on the current HS/LP clock
rates shouldn't be better to calculate these two here? Instead of doing
 this in the DSI host driver? This way all the DPHY specific
calculations whould in one place.

> +
> +	/*
> +	 * hs_zero: forumula from NXP BSP
> +	 */
> +	n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) /
> 10000;
> +	cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> +
> +	/*
> +	 * clk_zero: forumula from NXP BSP
> +	 */
> +	n = (34 * (dphy_opts->hs_clk_rate / 1000000) - 2500) / 1000;
> +	cfg->mc_prg_hs_zero = n < 1 ? 1 : n;
> +
> +	/*
> +	 * clk_trail, hs_trail: forumula from NXP BSP
> +	 */
> +	n = (103 * (dphy_opts->hs_clk_rate / 1000000) + 10000) /
> 10000;
> +	if (n > 15)
> +		n = 15;
> +	if (n < 1)
> +		n = 1;
> +	cfg->m_prg_hs_trail = n;
> +	cfg->mc_prg_hs_trail = n;
> +
> +	/*
> +	 * rxhs_settle: formula from NXP BSP
> +	 */
> +	if (dphy_opts->hs_clk_rate < MBPS(80))
> +		cfg->rxhs_settle = 0x0d;
> +	else if (dphy_opts->hs_clk_rate < MBPS(90))
> +		cfg->rxhs_settle = 0x0c;
> +	else if (dphy_opts->hs_clk_rate < MBPS(125))
> +		cfg->rxhs_settle = 0x0b;
> +	else if (dphy_opts->hs_clk_rate < MBPS(150))
> +		cfg->rxhs_settle = 0x0a;
> +	else if (dphy_opts->hs_clk_rate < MBPS(225))
> +		cfg->rxhs_settle = 0x09;
> +	else if (dphy_opts->hs_clk_rate < MBPS(500))
> +		cfg->rxhs_settle = 0x08;
> +	else
> +		cfg->rxhs_settle = 0x07;
> +
> +	dev_dbg(&phy->dev, "hs_prepare: %u, clk_prepare: %u, "
> +		"hs_prepare: %u, clk_prepare: %u, "
> +		"hs_trail: %u, clk_trail: %u, "
> +		"rxhs_settle: %u\n",
> +		cfg->m_prg_hs_prepare, cfg->mc_prg_hs_prepare,
> +		cfg->m_prg_hs_zero, cfg->mc_prg_hs_zero,
> +		cfg->m_prg_hs_trail, cfg->mc_prg_hs_trail,
> +		cfg->rxhs_settle);
> +
> +	return 0;
> +}
> +
> +static void mixel_phy_set_hs_timings(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +
> +	phy_write(phy, priv->cfg.m_prg_hs_prepare,
> DPHY_M_PRG_HS_PREPARE);
> +	phy_write(phy, priv->cfg.mc_prg_hs_prepare,
> DPHY_MC_PRG_HS_PREPARE);
> +	phy_write(phy, priv->cfg.m_prg_hs_zero, DPHY_M_PRG_HS_ZERO);
> +	phy_write(phy, priv->cfg.mc_prg_hs_zero,
> DPHY_MC_PRG_HS_ZERO);
> +	phy_write(phy, priv->cfg.m_prg_hs_trail,
> DPHY_M_PRG_HS_TRAIL);
> +	phy_write(phy, priv->cfg.mc_prg_hs_trail,
> DPHY_MC_PRG_HS_TRAIL);
> +	phy_write(phy, priv->cfg.rxhs_settle, priv->devdata-
> >reg_rxhs_settle);
> +}
> +
> +static int mixel_dphy_set_pll_params(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> >dev.parent);
> +
> +	if (priv->cfg.cm < 16 || priv->cfg.cm > 255 ||
> +	    priv->cfg.cn < 1 || priv->cfg.cn > 32 ||
> +	    priv->cfg.co < 1 || priv->cfg.co > 8) {
> +		dev_err(&phy->dev, "Invalid CM/CN/CO values!
> (%u/%u/%u)\n",
> +			priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> +		return -EINVAL;
> +	}
> +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> +	phy_write(phy, CM(priv->cfg.cm), DPHY_CM);
> +	phy_write(phy, CN(priv->cfg.cn), DPHY_CN);
> +	phy_write(phy, CO(priv->cfg.co), DPHY_CO);
> +	return 0;
> +}
> +
> +static int mixel_dphy_configure(struct phy *phy, union
> phy_configure_opts *opts)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	struct mixel_dphy_cfg cfg = { 0 };
> +	int ret;
> +
> +	ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> &cfg);
> +	if (ret)
> +		return ret;
> +
> +	/* Update the configuration */
> +	memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> +
> +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> +
> +	phy_write(phy, 0x00, DPHY_LOCK_BYP);
> +	phy_write(phy, 0x01, priv->devdata->reg_tx_rcal);
> +	phy_write(phy, 0x00, priv->devdata->reg_auto_pd_en);
> +	phy_write(phy, 0x02, priv->devdata->reg_rxlprp);
> +	phy_write(phy, 0x02, priv->devdata->reg_rxcdrp);
> +	phy_write(phy, 0x25, DPHY_TST);
> +
> +	mixel_phy_set_hs_timings(phy);
> +	ret = mixel_dphy_set_pll_params(phy);
> +	if (ret < 0)
> +		return ret;
> +
> +	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)
> +		return -EINVAL;
> +
> +	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> &cfg);
> +}
> +
> +static int mixel_dphy_power_on(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	u32 locked;
> +
> +	clk_prepare_enable(priv->phy_ref_clk);
> +
> +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> +	phy_write(phy, PWR_ON, DPHY_PD_PLL);

According to the Mixel RM, the power on sequence is the following:
1. Deassert PD_PLL
2. Wait for DPHY_LOCK
3. Deassert PD_DPHY
Therefore, the PD_DPHY deassertion should be moved after the read_poll

Another thing regarding the power-on sequence: PD_PLL and PD_DPHY
states must be asserted before powering-on. In the NXP tree this was
done in the init stage, but this stage is missing here. I think you
could add this, or you could assert them (write PWR_OFF) in configure
stage.

> +
> +	if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
> +				     locked, 10, 1000) < 0) {
> +		dev_err(&phy->dev, "Could not get DPHY lock!\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mixel_dphy_power_off(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +
> +	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> +	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> +
> +	clk_disable_unprepare(priv->phy_ref_clk);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops mixel_dphy_phy_ops = {
> +	.power_on = mixel_dphy_power_on,
> +	.power_off = mixel_dphy_power_off,
> +	.configure = mixel_dphy_configure,
> +	.validate = mixel_dphy_validate,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id mixel_dphy_of_match[] = {
> +	{ .compatible = "mixel,imx8mq-mipi-dphy",
> +	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> +
> +static int mixel_dphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct phy_provider *phy_provider;
> +	struct mixel_dphy_priv *priv;
> +	struct resource *res;
> +	struct phy *phy;
> +	void __iomem *regs;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->devdata = of_device_get_match_data(&pdev->dev);
> +	if (!priv->devdata)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	regs = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(priv->regs)) {
> +		dev_err(dev, "Couldn't map the DPHY registers\n");
> +		return PTR_ERR(priv->regs);
> +	}
> +
> +	priv->regs = devm_regmap_init_mmio(&pdev->dev, regs,
> +					   &mixel_dphy_regmap_config
> );
> +	if (IS_ERR(priv->regs)) {
> +		dev_err(dev, "Couldn't create the DPHY regmap\n");
> +		return PTR_ERR(priv->regs);
> +	}
> +
> +	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> +	if (IS_ERR(priv->phy_ref_clk)) {
> +		dev_err(dev, "No phy_ref clock found\n");
> +		return PTR_ERR(priv->phy_ref_clk);
> +	}
> +	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> +		clk_get_rate(priv->phy_ref_clk));
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "Failed to create phy %ld\n",
> PTR_ERR(phy));
> +		return PTR_ERR(phy);
> +	}
> +	phy_set_drvdata(phy, priv);
> +
> +	phy_provider = devm_of_phy_provider_register(dev,
> of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver mixel_dphy_driver = {
> +	.probe	= mixel_dphy_probe,
> +	.driver = {
> +		.name = "mixel-mipi-dphy",
> +		.of_match_table	= mixel_dphy_of_match,
> +	}
> +};
> +module_platform_driver(mixel_dphy_driver);
> +
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
> +MODULE_LICENSE("GPL v2");

Guido Günther Feb. 8, 2019, 11:38 a.m. UTC | #4

Hi,
On Fri, Feb 01, 2019 at 09:26:50AM -0200, Fabio Estevam wrote:
> Hi Guido,
> 
> Thanks for the respin. It looks better :-)

Thanks for having a look! All your comments made sense to me and I've
folded them into v3.
Cheers,
 -- Guido

> 
> On Fri, Feb 1, 2019 at 6:50 AM Guido Günther <[e-mail]> wrote:
> 
> > +config PHY_MIXEL_MIPI_DPHY
> > +       tristate "Mixel MIPI DSI PHY support"
> > +       depends on OF
> > +       select GENERIC_PHY
> > +       select GENERIC_PHY_MIPI_DPHY
> 
> Since you converted to regmap, I guess you need:
> select REGMAP_MMIO now?
> 
> > +       help
> > +         Enable this to add support for the Mixel DSI PHY as found
> > +         on NXP's i.MX8 family of SOCs.
> > diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
> > index dc2b3f1f2f80..07491c926a2c 100644
> > --- a/drivers/phy/freescale/Makefile
> > +++ b/drivers/phy/freescale/Makefile
> > @@ -1 +1,2 @@
> >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)       += phy-fsl-imx8mq-usb.o
> > +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)      += phy-fsl-imx8-mipi-dphy.o
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > new file mode 100644
> > index 000000000000..4b182f2eaa6e
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > @@ -0,0 +1,494 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2017,2018 NXP
> > + * Copyright 2019 Purism SPC
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/regmap.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* DPHY registers */
> > +#define DPHY_PD_DPHY                   0x00
> > +#define DPHY_M_PRG_HS_PREPARE          0x04
> > +#define DPHY_MC_PRG_HS_PREPARE         0x08
> > +#define DPHY_M_PRG_HS_ZERO             0x0c
> > +#define DPHY_MC_PRG_HS_ZERO            0x10
> > +#define DPHY_M_PRG_HS_TRAIL            0x14
> > +#define DPHY_MC_PRG_HS_TRAIL           0x18
> > +#define DPHY_PD_PLL                    0x1c
> > +#define DPHY_TST                       0x20
> > +#define DPHY_CN                                0x24
> > +#define DPHY_CM                                0x28
> > +#define DPHY_CO                                0x2c
> > +#define DPHY_LOCK                      0x30
> > +#define DPHY_LOCK_BYP                  0x34
> > +#define DPHY_REG_BYPASS_PLL            0x4C
> > +
> > +#define MBPS(x) ((x) * 1000000)
> > +
> > +#define DATA_RATE_MAX_SPEED MBPS(1500)
> > +#define DATA_RATE_MIN_SPEED MBPS(80)
> > +
> > +#define CN_BUF 0xcb7a89c0
> > +#define CO_BUF 0x63
> > +#define CM(x)  (                               \
> > +               ((x) <  32)?0xe0|((x)-16) :     \
> 
> Doesn't checkpatch complain about the need of space between operators?
> 
> > +               ((x) <  64)?0xc0|((x)-32) :     \
> > +               ((x) < 128)?0x80|((x)-64) :     \
> > +               ((x) - 128))
> > +#define CN(x)  (((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
> > +#define CO(x)  ((CO_BUF)>>(8-(x))&0x3)
> > +
> > +/* PHY power on is LOW_ENABLE */
> 
> active low is probably a better term.
> 
> > +#define PWR_ON 0
> > +#define PWR_OFF        1
> 
> > +static inline u32 phy_read(struct phy *phy, unsigned int reg)
> 
> After the conversion to regmap this function is unused now and you
> should remove it.
> 
> > +static inline void phy_write(struct phy *phy, u32 value, unsigned int reg)
> 
> No need for "inline".
> 
> Make it static int instead.
> 
> > +{
> > +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +       int ret;
> > +
> > +       ret = regmap_write(priv->regs, reg, value);
> 
> Or maybe get rid of this function and use regmap_write() instead.
> 
> > +       frequency = ref_clk * numerator / (2 * denominator);
> > +       dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
> 
> dev_dbg() would be better?
> 
> > +                frequency, dphy_opts->hs_clk_rate, ref_clk,
> > +                numerator, denominator);
> > +
> > +       /* LP clock period */
> > +       lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> > +       dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> > +               dphy_opts->lp_clk_rate, lp_t);
> > +       /*
> > +        *  hs_prepare: in lp clock periods
> > +        */
> 
> Please use single line comment style instead.
> 
> > +       if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> > +               dev_err(&phy->dev,
> > +                       "hs_prepare (%u) > 2.5 * lp clock period (%lu)",
> > +                       dphy_opts->hs_prepare, lp_t);
> > +               return -EINVAL;
> > +       }
> > +       /* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
> > +       if (dphy_opts->hs_prepare < lp_t)
> > +               n = 0;
> > +       else
> > +               n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> > +       cfg->m_prg_hs_prepare = n;
> > +
> > +       /*
> > +        * clk_prepare: in lp clock periods
> > +        */
> 
> Same here.
> 
> > +       if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> > +               dev_err(&phy->dev,
> > +                       "clk_prepare (%u) > 1.5 * lp clock period (%lu)",
> > +                       dphy_opts->clk_prepare, lp_t);
> > +               return -EINVAL;
> > +       }
> > +       /* 00: lp_t, 01: 1.5 * lp_t */
> > +       cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 : 0;
> > +
> > +       /*
> > +        * hs_zero: forumula from NXP BSP
> 
> Typo: formula
> 
> > +        */
> > +       n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) / 10000;
> > +       cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> > +
> > +       /*
> > +        * clk_zero: forumula from NXP BSP
> 
> Typo: formula
> 
> > +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +       struct mixel_dphy_cfg cfg = { 0 };
> > +       int ret;
> > +
> > +       ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Update the configuration */
> > +       memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> > +
> > +       dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> 
> You have already printed CM, CN, CO above. No need to printed again.
> 
> > +static int mixel_dphy_power_on(struct phy *phy)
> > +{
> > +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +       u32 locked;
> > +
> > +       clk_prepare_enable(priv->phy_ref_clk);
> 
> clk_prepare_enable() may fail. Better do:
> 
> ret = clk_prepare_enable(priv->phy_ref_clk);
> if (ret < 0)
>     return ret;
> 
> > +
> > +       phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > +       phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > +
> > +       if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
> > +                                    locked, 10, 1000) < 0) {
> > +               dev_err(&phy->dev, "Could not get DPHY lock!\n");
> > +               return -EINVAL;
> 
> Please add defines for 10 and 1000.
> 
> Better propagate the real error value here:
> 
> ret = regmap_read_poll_timeout(...)
> if (ret) {
>    dev_err();
>    goto clock_disable
> }
> 
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res)
> > +               return -ENODEV;
> 
> You can remove the NULL check ...
> 
> > +
> > +       regs = devm_ioremap(dev, res->start, resource_size(res));
> 
> If you use devm_ioremap_resource() instead.
>

Guido Günther Feb. 8, 2019, 11:40 a.m. UTC | #5

Hi Robert,
On Wed, Feb 06, 2019 at 03:28:07PM +0000, Robert Chiras wrote:
> Hi Guido,
> 
> Thanks for picking this up. It's interesting to see that a lot has
> changed in the PHY API and the phy can be now configured through the
> API instead of exported function as I did in the NXP tree.
> 
> I was going through your implementation and I noticed you also added
> the phy_ref clock to this driver too. This is good, since the DPHY
> needs this clock, but I have a question related to the other clocks:
> According to the Northwest Logic reference manual (the DSI host that
> uses this DPHY), the host relies on the TX clock in order to configure
> the DPHY. Is this driver relying on it's user to also enable the TX
> clock?

Yes, I think that would be best. In fact due to lack of reference
manuals for nwl and mixel I didn't even know exactly which clocks needed
to be on already so I currently set for enabling this after the nwl
clocks. Are these manuals available publicly somewhere, I couldn't find
them?

> Also: did you test this driver? Because I have a version of the patches
> from NXP tree rebased on top of latest linux-next and I have a working

Hmm...could you (maybe off list) send the boot output with DEBUG 1
at the top of the driver and drm.debug=0x2f on the kernel command line?
Maybe I can spot something.

> version of eLCDIF with Raydium RM67191 DSI panel on mScale850D
> (i.MX8MQ). And I tried using this driver but there is no signal on the
> screen, even through the register values are all identical. Next, I'll
> try to debug why isn't this working on my setup.

I'm testing this on the Librem 5 devkit with a rockchip panel atm using
DCSS not eLCDIF though. My plan is to move to the NXP evk in the not so
far future to make this easier to reproduce.

> Other than the above questions, I have some suggestions inline.
> 
> Best regards,
> Robert
> 
> 
> On Vi, 2019-02-01 at 09:49 +0100, Guido Günther wrote:
> > This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
> > this is an IP core it will likely be found on others in the future.
> > So
> > instead of adding this to the nwl host driver make it a generic PHY
> > driver.
> > 
> > The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can
> > be
> > added once the necessary system controller bits are in via
> > mixel_dphy_devdata.
> > 
> > Signed-off-by: Guido Günther <[e-mail]>
> > ---
> >  drivers/phy/freescale/Kconfig                 |   9 +
> >  drivers/phy/freescale/Makefile                |   1 +
> >  .../phy/freescale/phy-fsl-imx8-mipi-dphy.c    | 494
> > ++++++++++++++++++
> >  3 files changed, 504 insertions(+)
> >  create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > 
> > diff --git a/drivers/phy/freescale/Kconfig
> > b/drivers/phy/freescale/Kconfig
> > index 832670b4952b..43a5ca25d44b 100644
> > --- a/drivers/phy/freescale/Kconfig
> > +++ b/drivers/phy/freescale/Kconfig
> > @@ -3,3 +3,12 @@ config PHY_FSL_IMX8MQ_USB
> >  	depends on OF && HAS_IOMEM
> >  	select GENERIC_PHY
> >  	default ARCH_MXC && ARM64
> > +
> > +config PHY_MIXEL_MIPI_DPHY
> > +	tristate "Mixel MIPI DSI PHY support"
> > +	depends on OF
> > +	select GENERIC_PHY
> > +	select GENERIC_PHY_MIPI_DPHY
> > +	help
> > +	  Enable this to add support for the Mixel DSI PHY as found
> > +	  on NXP's i.MX8 family of SOCs.
> > diff --git a/drivers/phy/freescale/Makefile
> > b/drivers/phy/freescale/Makefile
> > index dc2b3f1f2f80..07491c926a2c 100644
> > --- a/drivers/phy/freescale/Makefile
> > +++ b/drivers/phy/freescale/Makefile
> > @@ -1 +1,2 @@
> >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
> > +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > new file mode 100644
> > index 000000000000..4b182f2eaa6e
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > @@ -0,0 +1,494 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2017,2018 NXP
> > + * Copyright 2019 Purism SPC
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/regmap.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* DPHY registers */
> > +#define DPHY_PD_DPHY			0x00
> > +#define DPHY_M_PRG_HS_PREPARE		0x04
> > +#define DPHY_MC_PRG_HS_PREPARE		0x08
> > +#define DPHY_M_PRG_HS_ZERO		0x0c
> > +#define DPHY_MC_PRG_HS_ZERO		0x10
> > +#define DPHY_M_PRG_HS_TRAIL		0x14
> > +#define DPHY_MC_PRG_HS_TRAIL		0x18
> > +#define DPHY_PD_PLL			0x1c
> > +#define DPHY_TST			0x20
> > +#define DPHY_CN				0x24
> > +#define DPHY_CM				0x28
> > +#define DPHY_CO				0x2c
> > +#define DPHY_LOCK			0x30
> > +#define DPHY_LOCK_BYP			0x34
> > +#define DPHY_REG_BYPASS_PLL		0x4C
> > +
> > +#define MBPS(x) ((x) * 1000000)
> > +
> > +#define DATA_RATE_MAX_SPEED MBPS(1500)
> > +#define DATA_RATE_MIN_SPEED MBPS(80)
> > +
> > +#define CN_BUF	0xcb7a89c0
> > +#define CO_BUF	0x63
> > +#define CM(x)	(				\
> > +		((x) <	32)?0xe0|((x)-16) :	\
> > +		((x) <	64)?0xc0|((x)-32) :	\
> > +		((x) < 128)?0x80|((x)-64) :	\
> > +		((x) - 128))
> > +#define CN(x)	(((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
> > +#define CO(x)	((CO_BUF)>>(8-(x))&0x3)
> > +
> > +/* PHY power on is LOW_ENABLE */
> > +#define PWR_ON	0
> > +#define PWR_OFF	1
> > +
> > +enum mixel_dphy_devtype {
> > +	MIXEL_IMX8MQ,
> > +};
> > +
> > +struct mixel_dphy_devdata {
> > +	u8 reg_tx_rcal;
> > +	u8 reg_auto_pd_en;
> > +	u8 reg_rxlprp;
> > +	u8 reg_rxcdrp;
> > +	u8 reg_rxhs_settle;
> > +};
> > +
> > +static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > +	[MIXEL_IMX8MQ] = {
> > +		.reg_tx_rcal = 0x38,
> > +		.reg_auto_pd_en = 0x3c,
> > +		.reg_rxlprp = 0x40,
> > +		.reg_rxcdrp = 0x44,
> > +		.reg_rxhs_settle = 0x48,
> > +	},
> > +};
> > +
> > +struct mixel_dphy_cfg {
> > +	/* DPHY PLL parameters */
> > +	u32 cm;
> > +	u32 cn;
> > +	u32 co;
> > +	/* DPHY register values */
> > +	u8 mc_prg_hs_prepare;
> > +	u8 m_prg_hs_prepare;
> > +	u8 mc_prg_hs_zero;
> > +	u8 m_prg_hs_zero;
> > +	u8 mc_prg_hs_trail;
> > +	u8 m_prg_hs_trail;
> > +	u8 rxhs_settle;
> > +};
> > +
> > +struct mixel_dphy_priv {
> > +	struct mixel_dphy_cfg cfg;
> > +	struct regmap *regs;
> > +	struct clk *phy_ref_clk;
> > +	const struct mixel_dphy_devdata *devdata;
> > +};
> > +
> > +static const struct regmap_config mixel_dphy_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.max_register = DPHY_REG_BYPASS_PLL,
> > +	.name = "mipi-dphy",
> > +};
> > +
> > +static inline u32 phy_read(struct phy *phy, unsigned int reg)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regs, reg, &val);
> > +	if (ret < 0)
> > +		dev_err(&phy->dev, "Failed to read DPHY reg %d: %d",
> > reg, ret);
> > +	return val;
> > +}
> > +
> > +static inline void phy_write(struct phy *phy, u32 value, unsigned
> > int reg)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	int ret;
> > +
> > +	ret = regmap_write(priv->regs, reg, value);
> > +	if (ret < 0)
> > +		dev_err(&phy->dev, "Failed to write DPHY reg %d:
> > %d", reg, ret);
> > +}
> > +
> > +/*
> > + * Find a ratio close to the desired one using continued fraction
> > + * approximation ending either at exact match or maximum allowed
> > + * nominator, denominator.
> > + */
> > +static void get_best_ratio(unsigned long *pnum, unsigned long
> > *pdenom,
> > +			   unsigned int max_n, unsigned int max_d)
> > +{
> > +	unsigned long a = *pnum;
> > +	unsigned long b = *pdenom;
> > +	unsigned long c;
> > +	unsigned int n[] = {0, 1};
> > +	unsigned int d[] = {1, 0};
> > +	unsigned int whole;
> > +	unsigned int i = 1;
> > +
> > +	while (b) {
> > +		i ^= 1;
> > +		whole = a / b;
> > +		n[i] += (n[i ^ 1] * whole);
> > +		d[i] += (d[i ^ 1] * whole);
> > +		if ((n[i] > max_n) || (d[i] > max_d)) {
> > +			i ^= 1;
> > +			break;
> > +		}
> > +		c = a - (b * whole);
> > +		a = b;
> > +		b = c;
> > +	}
> > +	*pnum = n[i];
> > +	*pdenom = d[i];
> > +}
> > +
> > +static int mixel_dphy_config_from_opts(struct phy *phy,
> > +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> > +	       struct mixel_dphy_cfg *cfg)
> > +{
> > +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> > >dev.parent);
> > +	unsigned long ref_clk = clk_get_rate(priv->phy_ref_clk);
> > +	unsigned long lp_t, numerator, denominator, frequency;
> > +	u32 n;
> > +	int i;
> > +
> > +	if (dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED ||
> > +	    dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED)
> > +		return -EINVAL;
> > +
> > +	numerator = dphy_opts->hs_clk_rate;
> > +	denominator = ref_clk;
> > +	get_best_ratio(&numerator, &denominator, 255, 256);
> > +	if (!numerator || !denominator) {
> > +		dev_err(&phy->dev, "Invalid %ld/%ld for %ld/%ld\n",
> > +			numerator, denominator,
> > +			dphy_opts->hs_clk_rate, ref_clk);
> > +		return -EINVAL;
> > +	}
> > +
> > +	while ((numerator < 16) && (denominator <= 128)) {
> > +		numerator <<= 1;
> > +		denominator <<= 1;
> > +	}
> > +	/*
> > +	 * CM ranges between 16 and 255
> > +	 * CN ranges between 1 and 32
> > +	 * CO is power of 2: 1, 2, 4, 8
> > +	 */
> > +	i = __ffs(denominator);
> > +	if (i > 3)
> > +		i = 3;
> > +	cfg->cn = denominator >> i;
> > +	cfg->co = 1 << i;
> > +	cfg->cm = numerator;
> > +
> > +	if (cfg->cm < 16 || cfg->cm > 255 ||
> > +	    cfg->cn < 1 || cfg->cn > 32 ||
> > +	    cfg->co < 1 || cfg->co > 8) {
> > +		dev_err(&phy->dev, "Invalid CM/CN/CO values:
> > %u/%u/%u\n",
> > +			cfg->cm, cfg->cn, cfg->co);
> > +		dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰
> > %ld/%ld\n",
> > +			dphy_opts->hs_clk_rate, ref_clk,
> > +			numerator, denominator);
> > +		return -EINVAL;
> > +	}
> > +
> > +	frequency = ref_clk * numerator / (2 * denominator);
> > +	dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰
> > %ld/%ld\n",
> > +		 frequency, dphy_opts->hs_clk_rate, ref_clk,
> > +		 numerator, denominator);
> > +
> > +	/* LP clock period */
> > +	lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> > +	dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> > +		dphy_opts->lp_clk_rate, lp_t);
> > +	/*
> > +	 *  hs_prepare: in lp clock periods
> > +	 */
> > +	if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> > +		dev_err(&phy->dev,
> > +			"hs_prepare (%u) > 2.5 * lp clock period
> > (%lu)",
> > +			dphy_opts->hs_prepare, lp_t);
> > +		return -EINVAL;
> > +	}
> > +	/* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
> > +	if (dphy_opts->hs_prepare < lp_t)
> > +		n = 0;
> > +	else
> > +		n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> > +	cfg->m_prg_hs_prepare = n;
> > +
> > +	/*
> > +	 * clk_prepare: in lp clock periods
> > +	 */
> > +	if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> > +		dev_err(&phy->dev,
> > +			"clk_prepare (%u) > 1.5 * lp clock period
> > (%lu)",
> > +			dphy_opts->clk_prepare, lp_t);
> > +		return -EINVAL;
> > +	}
> > +	/* 00: lp_t, 01: 1.5 * lp_t */
> > +	cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 :
> > 0;
> 
> Since hs_prepare and clk_prepare both depend on the current HS/LP clock
> rates shouldn't be better to calculate these two here? Instead of doing
>  this in the DSI host driver? This way all the DPHY specific
> calculations whould in one place.

The idea of doing this in the host driver was that the host driver knows
about any special timing requirements from e.g. the panel so it can
adjust values as needed and the PHYs job would be to adhere to these
values. Does that make sense to you too?

> > +
> > +	/*
> > +	 * hs_zero: forumula from NXP BSP
> > +	 */
> > +	n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) /
> > 10000;
> > +	cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> > +
> > +	/*
> > +	 * clk_zero: forumula from NXP BSP
> > +	 */
> > +	n = (34 * (dphy_opts->hs_clk_rate / 1000000) - 2500) / 1000;
> > +	cfg->mc_prg_hs_zero = n < 1 ? 1 : n;
> > +
> > +	/*
> > +	 * clk_trail, hs_trail: forumula from NXP BSP
> > +	 */
> > +	n = (103 * (dphy_opts->hs_clk_rate / 1000000) + 10000) /
> > 10000;
> > +	if (n > 15)
> > +		n = 15;
> > +	if (n < 1)
> > +		n = 1;
> > +	cfg->m_prg_hs_trail = n;
> > +	cfg->mc_prg_hs_trail = n;
> > +
> > +	/*
> > +	 * rxhs_settle: formula from NXP BSP
> > +	 */
> > +	if (dphy_opts->hs_clk_rate < MBPS(80))
> > +		cfg->rxhs_settle = 0x0d;
> > +	else if (dphy_opts->hs_clk_rate < MBPS(90))
> > +		cfg->rxhs_settle = 0x0c;
> > +	else if (dphy_opts->hs_clk_rate < MBPS(125))
> > +		cfg->rxhs_settle = 0x0b;
> > +	else if (dphy_opts->hs_clk_rate < MBPS(150))
> > +		cfg->rxhs_settle = 0x0a;
> > +	else if (dphy_opts->hs_clk_rate < MBPS(225))
> > +		cfg->rxhs_settle = 0x09;
> > +	else if (dphy_opts->hs_clk_rate < MBPS(500))
> > +		cfg->rxhs_settle = 0x08;
> > +	else
> > +		cfg->rxhs_settle = 0x07;
> > +
> > +	dev_dbg(&phy->dev, "hs_prepare: %u, clk_prepare: %u, "
> > +		"hs_prepare: %u, clk_prepare: %u, "
> > +		"hs_trail: %u, clk_trail: %u, "
> > +		"rxhs_settle: %u\n",
> > +		cfg->m_prg_hs_prepare, cfg->mc_prg_hs_prepare,
> > +		cfg->m_prg_hs_zero, cfg->mc_prg_hs_zero,
> > +		cfg->m_prg_hs_trail, cfg->mc_prg_hs_trail,
> > +		cfg->rxhs_settle);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mixel_phy_set_hs_timings(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +
> > +	phy_write(phy, priv->cfg.m_prg_hs_prepare,
> > DPHY_M_PRG_HS_PREPARE);
> > +	phy_write(phy, priv->cfg.mc_prg_hs_prepare,
> > DPHY_MC_PRG_HS_PREPARE);
> > +	phy_write(phy, priv->cfg.m_prg_hs_zero, DPHY_M_PRG_HS_ZERO);
> > +	phy_write(phy, priv->cfg.mc_prg_hs_zero,
> > DPHY_MC_PRG_HS_ZERO);
> > +	phy_write(phy, priv->cfg.m_prg_hs_trail,
> > DPHY_M_PRG_HS_TRAIL);
> > +	phy_write(phy, priv->cfg.mc_prg_hs_trail,
> > DPHY_MC_PRG_HS_TRAIL);
> > +	phy_write(phy, priv->cfg.rxhs_settle, priv->devdata-
> > >reg_rxhs_settle);
> > +}
> > +
> > +static int mixel_dphy_set_pll_params(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> > >dev.parent);
> > +
> > +	if (priv->cfg.cm < 16 || priv->cfg.cm > 255 ||
> > +	    priv->cfg.cn < 1 || priv->cfg.cn > 32 ||
> > +	    priv->cfg.co < 1 || priv->cfg.co > 8) {
> > +		dev_err(&phy->dev, "Invalid CM/CN/CO values!
> > (%u/%u/%u)\n",
> > +			priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> > +		return -EINVAL;
> > +	}
> > +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> > +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> > +	phy_write(phy, CM(priv->cfg.cm), DPHY_CM);
> > +	phy_write(phy, CN(priv->cfg.cn), DPHY_CN);
> > +	phy_write(phy, CO(priv->cfg.co), DPHY_CO);
> > +	return 0;
> > +}
> > +
> > +static int mixel_dphy_configure(struct phy *phy, union
> > phy_configure_opts *opts)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	struct mixel_dphy_cfg cfg = { 0 };
> > +	int ret;
> > +
> > +	ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > &cfg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Update the configuration */
> > +	memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> > +
> > +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> > +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> > +
> > +	phy_write(phy, 0x00, DPHY_LOCK_BYP);
> > +	phy_write(phy, 0x01, priv->devdata->reg_tx_rcal);
> > +	phy_write(phy, 0x00, priv->devdata->reg_auto_pd_en);
> > +	phy_write(phy, 0x02, priv->devdata->reg_rxlprp);
> > +	phy_write(phy, 0x02, priv->devdata->reg_rxcdrp);
> > +	phy_write(phy, 0x25, DPHY_TST);
> > +
> > +	mixel_phy_set_hs_timings(phy);
> > +	ret = mixel_dphy_set_pll_params(phy);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	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)
> > +		return -EINVAL;
> > +
> > +	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > &cfg);
> > +}
> > +
> > +static int mixel_dphy_power_on(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +	u32 locked;
> > +
> > +	clk_prepare_enable(priv->phy_ref_clk);
> > +
> > +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> 
> According to the Mixel RM, the power on sequence is the following:
> 1. Deassert PD_PLL
> 2. Wait for DPHY_LOCK
> 3. Deassert PD_DPHY
> Therefore, the PD_DPHY deassertion should be moved after the read_poll
> 
> Another thing regarding the power-on sequence: PD_PLL and PD_DPHY
> states must be asserted before powering-on. In the NXP tree this was
> done in the init stage, but this stage is missing here. I think you
> could add this, or you could assert them (write PWR_OFF) in configure
> stage.

I've folded both of these in for v3. These things were done differently
in the NXP 4.9 driver where I picked that form, I also added an exit
function to reset CN/CM/CO as is done in NXPs 4.14 driver.
Thanks for having a look,
 -- Guido

> 
> > +
> > +	if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
> > +				     locked, 10, 1000) < 0) {
> > +		dev_err(&phy->dev, "Could not get DPHY lock!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mixel_dphy_power_off(struct phy *phy)
> > +{
> > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > +
> > +	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> > +	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> > +
> > +	clk_disable_unprepare(priv->phy_ref_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct phy_ops mixel_dphy_phy_ops = {
> > +	.power_on = mixel_dphy_power_on,
> > +	.power_off = mixel_dphy_power_off,
> > +	.configure = mixel_dphy_configure,
> > +	.validate = mixel_dphy_validate,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id mixel_dphy_of_match[] = {
> > +	{ .compatible = "mixel,imx8mq-mipi-dphy",
> > +	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> > +
> > +static int mixel_dphy_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct phy_provider *phy_provider;
> > +	struct mixel_dphy_priv *priv;
> > +	struct resource *res;
> > +	struct phy *phy;
> > +	void __iomem *regs;
> > +
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->devdata = of_device_get_match_data(&pdev->dev);
> > +	if (!priv->devdata)
> > +		return -EINVAL;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +
> > +	regs = devm_ioremap(dev, res->start, resource_size(res));
> > +	if (IS_ERR(priv->regs)) {
> > +		dev_err(dev, "Couldn't map the DPHY registers\n");
> > +		return PTR_ERR(priv->regs);
> > +	}
> > +
> > +	priv->regs = devm_regmap_init_mmio(&pdev->dev, regs,
> > +					   &mixel_dphy_regmap_config
> > );
> > +	if (IS_ERR(priv->regs)) {
> > +		dev_err(dev, "Couldn't create the DPHY regmap\n");
> > +		return PTR_ERR(priv->regs);
> > +	}
> > +
> > +	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> > +	if (IS_ERR(priv->phy_ref_clk)) {
> > +		dev_err(dev, "No phy_ref clock found\n");
> > +		return PTR_ERR(priv->phy_ref_clk);
> > +	}
> > +	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> > +		clk_get_rate(priv->phy_ref_clk));
> > +
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> > +	if (IS_ERR(phy)) {
> > +		dev_err(dev, "Failed to create phy %ld\n",
> > PTR_ERR(phy));
> > +		return PTR_ERR(phy);
> > +	}
> > +	phy_set_drvdata(phy, priv);
> > +
> > +	phy_provider = devm_of_phy_provider_register(dev,
> > of_phy_simple_xlate);
> > +
> > +	return PTR_ERR_OR_ZERO(phy_provider);
> > +}
> > +
> > +static struct platform_driver mixel_dphy_driver = {
> > +	.probe	= mixel_dphy_probe,
> > +	.driver = {
> > +		.name = "mixel-mipi-dphy",
> > +		.of_match_table	= mixel_dphy_of_match,
> > +	}
> > +};
> > +module_platform_driver(mixel_dphy_driver);
> > +
> > +MODULE_AUTHOR("NXP Semiconductor");
> > +MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
> > +MODULE_LICENSE("GPL v2");

Robert Chiras Feb. 8, 2019, 11:55 a.m. UTC | #6

Hi Guido

On Vi, 2019-02-08 at 12:40 +0100, Guido Günther wrote:
> Hi Robert,
> On Wed, Feb 06, 2019 at 03:28:07PM +0000, Robert Chiras wrote:
> > 
> > Hi Guido,
> > 
> > Thanks for picking this up. It's interesting to see that a lot has
> > changed in the PHY API and the phy can be now configured through
> > the
> > API instead of exported function as I did in the NXP tree.
> > 
> > I was going through your implementation and I noticed you also
> > added
> > the phy_ref clock to this driver too. This is good, since the DPHY
> > needs this clock, but I have a question related to the other
> > clocks:
> > According to the Northwest Logic reference manual (the DSI host
> > that
> > uses this DPHY), the host relies on the TX clock in order to
> > configure
> > the DPHY. Is this driver relying on it's user to also enable the TX
> > clock?
> Yes, I think that would be best. In fact due to lack of reference
> manuals for nwl and mixel I didn't even know exactly which clocks
> needed
> to be on already so I currently set for enabling this after the nwl
> clocks. Are these manuals available publicly somewhere, I couldn't
> find
> them?

That's OK, I guess. Regarding the manuals: we have them from the vendor
so I can't share them.

> 
> > 
> > Also: did you test this driver? Because I have a version of the
> > patches
> > from NXP tree rebased on top of latest linux-next and I have a
> > working
> Hmm...could you (maybe off list) send the boot output with DEBUG 1
> at the top of the driver and drm.debug=0x2f on the kernel command
> line?
> Maybe I can spot something.

Eventually I got it working. On i.MX8MQ there is a System Reset
Controller that controls the clocks on each individual block. For some
reason, before asserting the MIPI clock domain in this SRC, a delay is
needed (right now, the hack is a sleep). Probably there is a component
that is not ready yet. Right now I am trying to figure out which one is
it and how can I wait for it.

> 
> > 
> > version of eLCDIF with Raydium RM67191 DSI panel on mScale850D
> > (i.MX8MQ). And I tried using this driver but there is no signal on
> > the
> > screen, even through the register values are all identical. Next,
> > I'll
> > try to debug why isn't this working on my setup.
> I'm testing this on the Librem 5 devkit with a rockchip panel atm
> using
> DCSS not eLCDIF though. My plan is to move to the NXP evk in the not
> so
> far future to make this easier to reproduce.

Good to know. Currently I am working on the eLCDIF pipeline on 850D to
make it ready for upstream. Since you took my DPHY driver and submitted
upstream in a better shape, I will make use of it.

> 
> > 
> > Other than the above questions, I have some suggestions inline.
> > 
> > Best regards,
> > Robert
> > 
> > 
> > On Vi, 2019-02-01 at 09:49 +0100, Guido Günther wrote:
> > > 
> > > This adds support for the Mixel DPHY as found on i.MX8 CPUs but
> > > since
> > > this is an IP core it will likely be found on others in the
> > > future.
> > > So
> > > instead of adding this to the nwl host driver make it a generic
> > > PHY
> > > driver.
> > > 
> > > The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP
> > > can
> > > be
> > > added once the necessary system controller bits are in via
> > > mixel_dphy_devdata.
> > > 
> > > Signed-off-by: Guido Günther <[e-mail]>
> > > ---
> > >  drivers/phy/freescale/Kconfig                 |   9 +
> > >  drivers/phy/freescale/Makefile                |   1 +
> > >  .../phy/freescale/phy-fsl-imx8-mipi-dphy.c    | 494
> > > ++++++++++++++++++
> > >  3 files changed, 504 insertions(+)
> > >  create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-
> > > dphy.c
> > > 
> > > diff --git a/drivers/phy/freescale/Kconfig
> > > b/drivers/phy/freescale/Kconfig
> > > index 832670b4952b..43a5ca25d44b 100644
> > > --- a/drivers/phy/freescale/Kconfig
> > > +++ b/drivers/phy/freescale/Kconfig
> > > @@ -3,3 +3,12 @@ config PHY_FSL_IMX8MQ_USB
> > >  	depends on OF && HAS_IOMEM
> > >  	select GENERIC_PHY
> > >  	default ARCH_MXC && ARM64
> > > +
> > > +config PHY_MIXEL_MIPI_DPHY
> > > +	tristate "Mixel MIPI DSI PHY support"
> > > +	depends on OF
> > > +	select GENERIC_PHY
> > > +	select GENERIC_PHY_MIPI_DPHY
> > > +	help
> > > +	  Enable this to add support for the Mixel DSI PHY as
> > > found
> > > +	  on NXP's i.MX8 family of SOCs.
> > > diff --git a/drivers/phy/freescale/Makefile
> > > b/drivers/phy/freescale/Makefile
> > > index dc2b3f1f2f80..07491c926a2c 100644
> > > --- a/drivers/phy/freescale/Makefile
> > > +++ b/drivers/phy/freescale/Makefile
> > > @@ -1 +1,2 @@
> > >  obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
> > > +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-
> > > dphy.o
> > > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > new file mode 100644
> > > index 000000000000..4b182f2eaa6e
> > > --- /dev/null
> > > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > > @@ -0,0 +1,494 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2017,2018 NXP
> > > + * Copyright 2019 Purism SPC
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/phy/phy.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +/* DPHY registers */
> > > +#define DPHY_PD_DPHY			0x00
> > > +#define DPHY_M_PRG_HS_PREPARE		0x04
> > > +#define DPHY_MC_PRG_HS_PREPARE		0x08
> > > +#define DPHY_M_PRG_HS_ZERO		0x0c
> > > +#define DPHY_MC_PRG_HS_ZERO		0x10
> > > +#define DPHY_M_PRG_HS_TRAIL		0x14
> > > +#define DPHY_MC_PRG_HS_TRAIL		0x18
> > > +#define DPHY_PD_PLL			0x1c
> > > +#define DPHY_TST			0x20
> > > +#define DPHY_CN				0x24
> > > +#define DPHY_CM				0x28
> > > +#define DPHY_CO				0x2c
> > > +#define DPHY_LOCK			0x30
> > > +#define DPHY_LOCK_BYP			0x34
> > > +#define DPHY_REG_BYPASS_PLL		0x4C
> > > +
> > > +#define MBPS(x) ((x) * 1000000)
> > > +
> > > +#define DATA_RATE_MAX_SPEED MBPS(1500)
> > > +#define DATA_RATE_MIN_SPEED MBPS(80)
> > > +
> > > +#define CN_BUF	0xcb7a89c0
> > > +#define CO_BUF	0x63
> > > +#define CM(x)	(				\
> > > +		((x) <	32)?0xe0|((x)-16) :	\
> > > +		((x) <	64)?0xc0|((x)-32) :	\
> > > +		((x) < 128)?0x80|((x)-64) :	\
> > > +		((x) - 128))
> > > +#define CN(x)	(((x) == 1)?0x1f : (((CN_BUF)>>((x)-
> > > 1))&0x1f))
> > > +#define CO(x)	((CO_BUF)>>(8-(x))&0x3)
> > > +
> > > +/* PHY power on is LOW_ENABLE */
> > > +#define PWR_ON	0
> > > +#define PWR_OFF	1
> > > +
> > > +enum mixel_dphy_devtype {
> > > +	MIXEL_IMX8MQ,
> > > +};
> > > +
> > > +struct mixel_dphy_devdata {
> > > +	u8 reg_tx_rcal;
> > > +	u8 reg_auto_pd_en;
> > > +	u8 reg_rxlprp;
> > > +	u8 reg_rxcdrp;
> > > +	u8 reg_rxhs_settle;
> > > +};
> > > +
> > > +static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > > +	[MIXEL_IMX8MQ] = {
> > > +		.reg_tx_rcal = 0x38,
> > > +		.reg_auto_pd_en = 0x3c,
> > > +		.reg_rxlprp = 0x40,
> > > +		.reg_rxcdrp = 0x44,
> > > +		.reg_rxhs_settle = 0x48,
> > > +	},
> > > +};
> > > +
> > > +struct mixel_dphy_cfg {
> > > +	/* DPHY PLL parameters */
> > > +	u32 cm;
> > > +	u32 cn;
> > > +	u32 co;
> > > +	/* DPHY register values */
> > > +	u8 mc_prg_hs_prepare;
> > > +	u8 m_prg_hs_prepare;
> > > +	u8 mc_prg_hs_zero;
> > > +	u8 m_prg_hs_zero;
> > > +	u8 mc_prg_hs_trail;
> > > +	u8 m_prg_hs_trail;
> > > +	u8 rxhs_settle;
> > > +};
> > > +
> > > +struct mixel_dphy_priv {
> > > +	struct mixel_dphy_cfg cfg;
> > > +	struct regmap *regs;
> > > +	struct clk *phy_ref_clk;
> > > +	const struct mixel_dphy_devdata *devdata;
> > > +};
> > > +
> > > +static const struct regmap_config mixel_dphy_regmap_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 32,
> > > +	.reg_stride = 4,
> > > +	.max_register = DPHY_REG_BYPASS_PLL,
> > > +	.name = "mipi-dphy",
> > > +};
> > > +
> > > +static inline u32 phy_read(struct phy *phy, unsigned int reg)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(priv->regs, reg, &val);
> > > +	if (ret < 0)
> > > +		dev_err(&phy->dev, "Failed to read DPHY reg %d:
> > > %d",
> > > reg, ret);
> > > +	return val;
> > > +}
> > > +
> > > +static inline void phy_write(struct phy *phy, u32 value,
> > > unsigned
> > > int reg)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(priv->regs, reg, value);
> > > +	if (ret < 0)
> > > +		dev_err(&phy->dev, "Failed to write DPHY reg %d:
> > > %d", reg, ret);
> > > +}
> > > +
> > > +/*
> > > + * Find a ratio close to the desired one using continued
> > > fraction
> > > + * approximation ending either at exact match or maximum allowed
> > > + * nominator, denominator.
> > > + */
> > > +static void get_best_ratio(unsigned long *pnum, unsigned long
> > > *pdenom,
> > > +			   unsigned int max_n, unsigned int
> > > max_d)
> > > +{
> > > +	unsigned long a = *pnum;
> > > +	unsigned long b = *pdenom;
> > > +	unsigned long c;
> > > +	unsigned int n[] = {0, 1};
> > > +	unsigned int d[] = {1, 0};
> > > +	unsigned int whole;
> > > +	unsigned int i = 1;
> > > +
> > > +	while (b) {
> > > +		i ^= 1;
> > > +		whole = a / b;
> > > +		n[i] += (n[i ^ 1] * whole);
> > > +		d[i] += (d[i ^ 1] * whole);
> > > +		if ((n[i] > max_n) || (d[i] > max_d)) {
> > > +			i ^= 1;
> > > +			break;
> > > +		}
> > > +		c = a - (b * whole);
> > > +		a = b;
> > > +		b = c;
> > > +	}
> > > +	*pnum = n[i];
> > > +	*pdenom = d[i];
> > > +}
> > > +
> > > +static int mixel_dphy_config_from_opts(struct phy *phy,
> > > +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> > > +	       struct mixel_dphy_cfg *cfg)
> > > +{
> > > +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> > > > 
> > > > dev.parent);
> > > +	unsigned long ref_clk = clk_get_rate(priv->phy_ref_clk);
> > > +	unsigned long lp_t, numerator, denominator, frequency;
> > > +	u32 n;
> > > +	int i;
> > > +
> > > +	if (dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED ||
> > > +	    dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED)
> > > +		return -EINVAL;
> > > +
> > > +	numerator = dphy_opts->hs_clk_rate;
> > > +	denominator = ref_clk;
> > > +	get_best_ratio(&numerator, &denominator, 255, 256);
> > > +	if (!numerator || !denominator) {
> > > +		dev_err(&phy->dev, "Invalid %ld/%ld for
> > > %ld/%ld\n",
> > > +			numerator, denominator,
> > > +			dphy_opts->hs_clk_rate, ref_clk);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	while ((numerator < 16) && (denominator <= 128)) {
> > > +		numerator <<= 1;
> > > +		denominator <<= 1;
> > > +	}
> > > +	/*
> > > +	 * CM ranges between 16 and 255
> > > +	 * CN ranges between 1 and 32
> > > +	 * CO is power of 2: 1, 2, 4, 8
> > > +	 */
> > > +	i = __ffs(denominator);
> > > +	if (i > 3)
> > > +		i = 3;
> > > +	cfg->cn = denominator >> i;
> > > +	cfg->co = 1 << i;
> > > +	cfg->cm = numerator;
> > > +
> > > +	if (cfg->cm < 16 || cfg->cm > 255 ||
> > > +	    cfg->cn < 1 || cfg->cn > 32 ||
> > > +	    cfg->co < 1 || cfg->co > 8) {
> > > +		dev_err(&phy->dev, "Invalid CM/CN/CO values:
> > > %u/%u/%u\n",
> > > +			cfg->cm, cfg->cn, cfg->co);
> > > +		dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰
> > > %ld/%ld\n",
> > > +			dphy_opts->hs_clk_rate, ref_clk,
> > > +			numerator, denominator);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	frequency = ref_clk * numerator / (2 * denominator);
> > > +	dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰
> > > %ld/%ld\n",
> > > +		 frequency, dphy_opts->hs_clk_rate, ref_clk,
> > > +		 numerator, denominator);
> > > +
> > > +	/* LP clock period */
> > > +	lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> > > +	dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> > > +		dphy_opts->lp_clk_rate, lp_t);
> > > +	/*
> > > +	 *  hs_prepare: in lp clock periods
> > > +	 */
> > > +	if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> > > +		dev_err(&phy->dev,
> > > +			"hs_prepare (%u) > 2.5 * lp clock period
> > > (%lu)",
> > > +			dphy_opts->hs_prepare, lp_t);
> > > +		return -EINVAL;
> > > +	}
> > > +	/* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 *
> > > lp_t */
> > > +	if (dphy_opts->hs_prepare < lp_t)
> > > +		n = 0;
> > > +	else
> > > +		n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> > > +	cfg->m_prg_hs_prepare = n;
> > > +
> > > +	/*
> > > +	 * clk_prepare: in lp clock periods
> > > +	 */
> > > +	if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> > > +		dev_err(&phy->dev,
> > > +			"clk_prepare (%u) > 1.5 * lp clock
> > > period
> > > (%lu)",
> > > +			dphy_opts->clk_prepare, lp_t);
> > > +		return -EINVAL;
> > > +	}
> > > +	/* 00: lp_t, 01: 1.5 * lp_t */
> > > +	cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ?
> > > 1 :
> > > 0;
> > Since hs_prepare and clk_prepare both depend on the current HS/LP
> > clock
> > rates shouldn't be better to calculate these two here? Instead of
> > doing
> >  this in the DSI host driver? This way all the DPHY specific
> > calculations whould in one place.
> The idea of doing this in the host driver was that the host driver
> knows
> about any special timing requirements from e.g. the panel so it can
> adjust values as needed and the PHYs job would be to adhere to these
> values. Does that make sense to you too?
> 
> > 
> > > 
> > > +
> > > +	/*
> > > +	 * hs_zero: forumula from NXP BSP
> > > +	 */
> > > +	n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) /
> > > 10000;
> > > +	cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> > > +
> > > +	/*
> > > +	 * clk_zero: forumula from NXP BSP
> > > +	 */
> > > +	n = (34 * (dphy_opts->hs_clk_rate / 1000000) - 2500) /
> > > 1000;
> > > +	cfg->mc_prg_hs_zero = n < 1 ? 1 : n;
> > > +
> > > +	/*
> > > +	 * clk_trail, hs_trail: forumula from NXP BSP
> > > +	 */
> > > +	n = (103 * (dphy_opts->hs_clk_rate / 1000000) + 10000) /
> > > 10000;
> > > +	if (n > 15)
> > > +		n = 15;
> > > +	if (n < 1)
> > > +		n = 1;
> > > +	cfg->m_prg_hs_trail = n;
> > > +	cfg->mc_prg_hs_trail = n;
> > > +
> > > +	/*
> > > +	 * rxhs_settle: formula from NXP BSP
> > > +	 */
> > > +	if (dphy_opts->hs_clk_rate < MBPS(80))
> > > +		cfg->rxhs_settle = 0x0d;
> > > +	else if (dphy_opts->hs_clk_rate < MBPS(90))
> > > +		cfg->rxhs_settle = 0x0c;
> > > +	else if (dphy_opts->hs_clk_rate < MBPS(125))
> > > +		cfg->rxhs_settle = 0x0b;
> > > +	else if (dphy_opts->hs_clk_rate < MBPS(150))
> > > +		cfg->rxhs_settle = 0x0a;
> > > +	else if (dphy_opts->hs_clk_rate < MBPS(225))
> > > +		cfg->rxhs_settle = 0x09;
> > > +	else if (dphy_opts->hs_clk_rate < MBPS(500))
> > > +		cfg->rxhs_settle = 0x08;
> > > +	else
> > > +		cfg->rxhs_settle = 0x07;
> > > +
> > > +	dev_dbg(&phy->dev, "hs_prepare: %u, clk_prepare: %u, "
> > > +		"hs_prepare: %u, clk_prepare: %u, "
> > > +		"hs_trail: %u, clk_trail: %u, "
> > > +		"rxhs_settle: %u\n",
> > > +		cfg->m_prg_hs_prepare, cfg->mc_prg_hs_prepare,
> > > +		cfg->m_prg_hs_zero, cfg->mc_prg_hs_zero,
> > > +		cfg->m_prg_hs_trail, cfg->mc_prg_hs_trail,
> > > +		cfg->rxhs_settle);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void mixel_phy_set_hs_timings(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +
> > > +	phy_write(phy, priv->cfg.m_prg_hs_prepare,
> > > DPHY_M_PRG_HS_PREPARE);
> > > +	phy_write(phy, priv->cfg.mc_prg_hs_prepare,
> > > DPHY_MC_PRG_HS_PREPARE);
> > > +	phy_write(phy, priv->cfg.m_prg_hs_zero,
> > > DPHY_M_PRG_HS_ZERO);
> > > +	phy_write(phy, priv->cfg.mc_prg_hs_zero,
> > > DPHY_MC_PRG_HS_ZERO);
> > > +	phy_write(phy, priv->cfg.m_prg_hs_trail,
> > > DPHY_M_PRG_HS_TRAIL);
> > > +	phy_write(phy, priv->cfg.mc_prg_hs_trail,
> > > DPHY_MC_PRG_HS_TRAIL);
> > > +	phy_write(phy, priv->cfg.rxhs_settle, priv->devdata-
> > > > 
> > > > reg_rxhs_settle);
> > > +}
> > > +
> > > +static int mixel_dphy_set_pll_params(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy-
> > > > 
> > > > dev.parent);
> > > +
> > > +	if (priv->cfg.cm < 16 || priv->cfg.cm > 255 ||
> > > +	    priv->cfg.cn < 1 || priv->cfg.cn > 32 ||
> > > +	    priv->cfg.co < 1 || priv->cfg.co > 8) {
> > > +		dev_err(&phy->dev, "Invalid CM/CN/CO values!
> > > (%u/%u/%u)\n",
> > > +			priv->cfg.cm, priv->cfg.cn, priv-
> > > >cfg.co);
> > > +		return -EINVAL;
> > > +	}
> > > +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> > > +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> > > +	phy_write(phy, CM(priv->cfg.cm), DPHY_CM);
> > > +	phy_write(phy, CN(priv->cfg.cn), DPHY_CN);
> > > +	phy_write(phy, CO(priv->cfg.co), DPHY_CO);
> > > +	return 0;
> > > +}
> > > +
> > > +static int mixel_dphy_configure(struct phy *phy, union
> > > phy_configure_opts *opts)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	struct mixel_dphy_cfg cfg = { 0 };
> > > +	int ret;
> > > +
> > > +	ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy,
> > > &cfg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Update the configuration */
> > > +	memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> > > +
> > > +	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
> > > +		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
> > > +
> > > +	phy_write(phy, 0x00, DPHY_LOCK_BYP);
> > > +	phy_write(phy, 0x01, priv->devdata->reg_tx_rcal);
> > > +	phy_write(phy, 0x00, priv->devdata->reg_auto_pd_en);
> > > +	phy_write(phy, 0x02, priv->devdata->reg_rxlprp);
> > > +	phy_write(phy, 0x02, priv->devdata->reg_rxcdrp);
> > > +	phy_write(phy, 0x25, DPHY_TST);
> > > +
> > > +	mixel_phy_set_hs_timings(phy);
> > > +	ret = mixel_dphy_set_pll_params(phy);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	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)
> > > +		return -EINVAL;
> > > +
> > > +	return mixel_dphy_config_from_opts(phy, &opts-
> > > >mipi_dphy,
> > > &cfg);
> > > +}
> > > +
> > > +static int mixel_dphy_power_on(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +	u32 locked;
> > > +
> > > +	clk_prepare_enable(priv->phy_ref_clk);
> > > +
> > > +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> > > +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> > According to the Mixel RM, the power on sequence is the following:
> > 1. Deassert PD_PLL
> > 2. Wait for DPHY_LOCK
> > 3. Deassert PD_DPHY
> > Therefore, the PD_DPHY deassertion should be moved after the
> > read_poll
> > 
> > Another thing regarding the power-on sequence: PD_PLL and PD_DPHY
> > states must be asserted before powering-on. In the NXP tree this
> > was
> > done in the init stage, but this stage is missing here. I think you
> > could add this, or you could assert them (write PWR_OFF) in
> > configure
> > stage.
> I've folded both of these in for v3. These things were done
> differently
> in the NXP 4.9 driver where I picked that form, I also added an exit
> function to reset CN/CM/CO as is done in NXPs 4.14 driver.
> Thanks for having a look,
>  -- Guido
> 
> > 
> > 
> > > 
> > > +
> > > +	if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK,
> > > locked,
> > > +				     locked, 10, 1000) < 0) {
> > > +		dev_err(&phy->dev, "Could not get DPHY
> > > lock!\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mixel_dphy_power_off(struct phy *phy)
> > > +{
> > > +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > > +
> > > +	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> > > +	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> > > +
> > > +	clk_disable_unprepare(priv->phy_ref_clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct phy_ops mixel_dphy_phy_ops = {
> > > +	.power_on = mixel_dphy_power_on,
> > > +	.power_off = mixel_dphy_power_off,
> > > +	.configure = mixel_dphy_configure,
> > > +	.validate = mixel_dphy_validate,
> > > +	.owner = THIS_MODULE,
> > > +};
> > > +
> > > +static const struct of_device_id mixel_dphy_of_match[] = {
> > > +	{ .compatible = "mixel,imx8mq-mipi-dphy",
> > > +	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
> > > +	{ /* sentinel */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> > > +
> > > +static int mixel_dphy_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	struct phy_provider *phy_provider;
> > > +	struct mixel_dphy_priv *priv;
> > > +	struct resource *res;
> > > +	struct phy *phy;
> > > +	void __iomem *regs;
> > > +
> > > +	if (!np)
> > > +		return -ENODEV;
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	priv->devdata = of_device_get_match_data(&pdev->dev);
> > > +	if (!priv->devdata)
> > > +		return -EINVAL;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res)
> > > +		return -ENODEV;
> > > +
> > > +	regs = devm_ioremap(dev, res->start,
> > > resource_size(res));
> > > +	if (IS_ERR(priv->regs)) {
> > > +		dev_err(dev, "Couldn't map the DPHY
> > > registers\n");
> > > +		return PTR_ERR(priv->regs);
> > > +	}
> > > +
> > > +	priv->regs = devm_regmap_init_mmio(&pdev->dev, regs,
> > > +					   &mixel_dphy_regmap_co
> > > nfig
> > > );
> > > +	if (IS_ERR(priv->regs)) {
> > > +		dev_err(dev, "Couldn't create the DPHY
> > > regmap\n");
> > > +		return PTR_ERR(priv->regs);
> > > +	}
> > > +
> > > +	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> > > +	if (IS_ERR(priv->phy_ref_clk)) {
> > > +		dev_err(dev, "No phy_ref clock found\n");
> > > +		return PTR_ERR(priv->phy_ref_clk);
> > > +	}
> > > +	dev_dbg(dev, "phy_ref clock rate: %lu\n",
> > > +		clk_get_rate(priv->phy_ref_clk));
> > > +
> > > +	dev_set_drvdata(dev, priv);
> > > +
> > > +	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
> > > +	if (IS_ERR(phy)) {
> > > +		dev_err(dev, "Failed to create phy %ld\n",
> > > PTR_ERR(phy));
> > > +		return PTR_ERR(phy);
> > > +	}
> > > +	phy_set_drvdata(phy, priv);
> > > +
> > > +	phy_provider = devm_of_phy_provider_register(dev,
> > > of_phy_simple_xlate);
> > > +
> > > +	return PTR_ERR_OR_ZERO(phy_provider);
> > > +}
> > > +
> > > +static struct platform_driver mixel_dphy_driver = {
> > > +	.probe	= mixel_dphy_probe,
> > > +	.driver = {
> > > +		.name = "mixel-mipi-dphy",
> > > +		.of_match_table	= mixel_dphy_of_match,
> > > +	}
> > > +};
> > > +module_platform_driver(mixel_dphy_driver);
> > > +
> > > +MODULE_AUTHOR("NXP Semiconductor");
> > > +MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
> > > +MODULE_LICENSE("GPL v2");

Guido Günther March 6, 2019, 1:55 p.m. UTC | #7

Hi,
On Fri, Feb 08, 2019 at 11:55:41AM +0000, Robert Chiras wrote:
> Hi Guido
> 
> On Vi, 2019-02-08 at 12:40 +0100, Guido Günther wrote:
> > Hi Robert,
> > On Wed, Feb 06, 2019 at 03:28:07PM +0000, Robert Chiras wrote:
> > > 
> > > Hi Guido,
> > > 
> > > Thanks for picking this up. It's interesting to see that a lot has
> > > changed in the PHY API and the phy can be now configured through
> > > the
> > > API instead of exported function as I did in the NXP tree.
> > > 
> > > I was going through your implementation and I noticed you also
> > > added
> > > the phy_ref clock to this driver too. This is good, since the DPHY
> > > needs this clock, but I have a question related to the other
> > > clocks:
> > > According to the Northwest Logic reference manual (the DSI host
> > > that
> > > uses this DPHY), the host relies on the TX clock in order to
> > > configure
> > > the DPHY. Is this driver relying on it's user to also enable the TX
> > > clock?
> > Yes, I think that would be best. In fact due to lack of reference
> > manuals for nwl and mixel I didn't even know exactly which clocks
> > needed
> > to be on already so I currently set for enabling this after the nwl
> > clocks. Are these manuals available publicly somewhere, I couldn't
> > find
> > them?
> 
> That's OK, I guess. Regarding the manuals: we have them from the vendor
> so I can't share them.

Too bad. Any contact I could ping there would also be nice?

> 
> > 
> > > 
> > > Also: did you test this driver? Because I have a version of the
> > > patches
> > > from NXP tree rebased on top of latest linux-next and I have a
> > > working
> > Hmm...could you (maybe off list) send the boot output with DEBUG 1
> > at the top of the driver and drm.debug=0x2f on the kernel command
> > line?
> > Maybe I can spot something.
> 
> Eventually I got it working. On i.MX8MQ there is a System Reset
> Controller that controls the clocks on each individual block. For some
> reason, before asserting the MIPI clock domain in this SRC, a delay is
> needed (right now, the hack is a sleep). Probably there is a component
> that is not ready yet. Right now I am trying to figure out which one is
> it and how can I wait for it.
> 
> > 
> > > 
> > > version of eLCDIF with Raydium RM67191 DSI panel on mScale850D
> > > (i.MX8MQ). And I tried using this driver but there is no signal on
> > > the
> > > screen, even through the register values are all identical. Next,
> > > I'll
> > > try to debug why isn't this working on my setup.
> > I'm testing this on the Librem 5 devkit with a rockchip panel atm
> > using
> > DCSS not eLCDIF though. My plan is to move to the NXP evk in the not
> > so
> > far future to make this easier to reproduce.
> 
> Good to know. Currently I am working on the eLCDIF pipeline on 850D to
> make it ready for upstream. Since you took my DPHY driver and submitted
> upstream in a better shape, I will make use of it.

Cool. I have an initial version of nwl mostly in shape now too (hope to
send it out in a couple of days). eLCDIF will be handy to test the
whole stack on 5.x.

Cheers,
 -- Guido

Guido Günther March 21, 2019, 5:12 p.m. UTC | #8

Hi,
On Fri, Feb 08, 2019 at 11:55:41AM +0000, Robert Chiras wrote:
[..snip..]
> > > version of eLCDIF with Raydium RM67191 DSI panel on mScale850D
> > > (i.MX8MQ). And I tried using this driver but there is no signal on
> > > the
> > > screen, even through the register values are all identical. Next,
> > > I'll
> > > try to debug why isn't this working on my setup.
> > I'm testing this on the Librem 5 devkit with a rockchip panel atm
> > using
> > DCSS not eLCDIF though. My plan is to move to the NXP evk in the not
> > so
> > far future to make this easier to reproduce.
> 
> Good to know. Currently I am working on the eLCDIF pipeline on 850D to
> make it ready for upstream. Since you took my DPHY driver and submitted
> upstream in a better shape, I will make use of it.

I've submitted a v6. In case you're already using it maybe adding a
Tested-By or even Reviewed-By: from you might help mainline adoption?

Cheers,
 -- Guido

Patch

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 832670b4952b..43a5ca25d44b 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -3,3 +3,12 @@  config PHY_FSL_IMX8MQ_USB
 	depends on OF && HAS_IOMEM
 	select GENERIC_PHY
 	default ARCH_MXC && ARM64
+
+config PHY_MIXEL_MIPI_DPHY
+	tristate "Mixel MIPI DSI PHY support"
+	depends on OF
+	select GENERIC_PHY
+	select GENERIC_PHY_MIPI_DPHY
+	help
+	  Enable this to add support for the Mixel DSI PHY as found
+	  on NXP's i.MX8 family of SOCs.
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index dc2b3f1f2f80..07491c926a2c 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
+obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
new file mode 100644
index 000000000000..4b182f2eaa6e
--- /dev/null
+++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
@@ -0,0 +1,494 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017,2018 NXP
+ * Copyright 2019 Purism SPC
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+
+/* DPHY registers */
+#define DPHY_PD_DPHY			0x00
+#define DPHY_M_PRG_HS_PREPARE		0x04
+#define DPHY_MC_PRG_HS_PREPARE		0x08
+#define DPHY_M_PRG_HS_ZERO		0x0c
+#define DPHY_MC_PRG_HS_ZERO		0x10
+#define DPHY_M_PRG_HS_TRAIL		0x14
+#define DPHY_MC_PRG_HS_TRAIL		0x18
+#define DPHY_PD_PLL			0x1c
+#define DPHY_TST			0x20
+#define DPHY_CN				0x24
+#define DPHY_CM				0x28
+#define DPHY_CO				0x2c
+#define DPHY_LOCK			0x30
+#define DPHY_LOCK_BYP			0x34
+#define DPHY_REG_BYPASS_PLL		0x4C
+
+#define MBPS(x) ((x) * 1000000)
+
+#define DATA_RATE_MAX_SPEED MBPS(1500)
+#define DATA_RATE_MIN_SPEED MBPS(80)
+
+#define CN_BUF	0xcb7a89c0
+#define CO_BUF	0x63
+#define CM(x)	(				\
+		((x) <	32)?0xe0|((x)-16) :	\
+		((x) <	64)?0xc0|((x)-32) :	\
+		((x) < 128)?0x80|((x)-64) :	\
+		((x) - 128))
+#define CN(x)	(((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
+#define CO(x)	((CO_BUF)>>(8-(x))&0x3)
+
+/* PHY power on is LOW_ENABLE */
+#define PWR_ON	0
+#define PWR_OFF	1
+
+enum mixel_dphy_devtype {
+	MIXEL_IMX8MQ,
+};
+
+struct mixel_dphy_devdata {
+	u8 reg_tx_rcal;
+	u8 reg_auto_pd_en;
+	u8 reg_rxlprp;
+	u8 reg_rxcdrp;
+	u8 reg_rxhs_settle;
+};
+
+static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
+	[MIXEL_IMX8MQ] = {
+		.reg_tx_rcal = 0x38,
+		.reg_auto_pd_en = 0x3c,
+		.reg_rxlprp = 0x40,
+		.reg_rxcdrp = 0x44,
+		.reg_rxhs_settle = 0x48,
+	},
+};
+
+struct mixel_dphy_cfg {
+	/* DPHY PLL parameters */
+	u32 cm;
+	u32 cn;
+	u32 co;
+	/* DPHY register values */
+	u8 mc_prg_hs_prepare;
+	u8 m_prg_hs_prepare;
+	u8 mc_prg_hs_zero;
+	u8 m_prg_hs_zero;
+	u8 mc_prg_hs_trail;
+	u8 m_prg_hs_trail;
+	u8 rxhs_settle;
+};
+
+struct mixel_dphy_priv {
+	struct mixel_dphy_cfg cfg;
+	struct regmap *regs;
+	struct clk *phy_ref_clk;
+	const struct mixel_dphy_devdata *devdata;
+};
+
+static const struct regmap_config mixel_dphy_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = DPHY_REG_BYPASS_PLL,
+	.name = "mipi-dphy",
+};
+
+static inline u32 phy_read(struct phy *phy, unsigned int reg)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(priv->regs, reg, &val);
+	if (ret < 0)
+		dev_err(&phy->dev, "Failed to read DPHY reg %d: %d", reg, ret);
+	return val;
+}
+
+static inline void phy_write(struct phy *phy, u32 value, unsigned int reg)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = regmap_write(priv->regs, reg, value);
+	if (ret < 0)
+		dev_err(&phy->dev, "Failed to write DPHY reg %d: %d", reg, ret);
+}
+
+/*
+ * Find a ratio close to the desired one using continued fraction
+ * approximation ending either at exact match or maximum allowed
+ * nominator, denominator.
+ */
+static void get_best_ratio(unsigned long *pnum, unsigned long *pdenom,
+			   unsigned int max_n, unsigned int max_d)
+{
+	unsigned long a = *pnum;
+	unsigned long b = *pdenom;
+	unsigned long c;
+	unsigned int n[] = {0, 1};
+	unsigned int d[] = {1, 0};
+	unsigned int whole;
+	unsigned int i = 1;
+
+	while (b) {
+		i ^= 1;
+		whole = a / b;
+		n[i] += (n[i ^ 1] * whole);
+		d[i] += (d[i ^ 1] * whole);
+		if ((n[i] > max_n) || (d[i] > max_d)) {
+			i ^= 1;
+			break;
+		}
+		c = a - (b * whole);
+		a = b;
+		b = c;
+	}
+	*pnum = n[i];
+	*pdenom = d[i];
+}
+
+static int mixel_dphy_config_from_opts(struct phy *phy,
+	       struct phy_configure_opts_mipi_dphy *dphy_opts,
+	       struct mixel_dphy_cfg *cfg)
+{
+	struct mixel_dphy_priv *priv = dev_get_drvdata(phy->dev.parent);
+	unsigned long ref_clk = clk_get_rate(priv->phy_ref_clk);
+	unsigned long lp_t, numerator, denominator, frequency;
+	u32 n;
+	int i;
+
+	if (dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED ||
+	    dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED)
+		return -EINVAL;
+
+	numerator = dphy_opts->hs_clk_rate;
+	denominator = ref_clk;
+	get_best_ratio(&numerator, &denominator, 255, 256);
+	if (!numerator || !denominator) {
+		dev_err(&phy->dev, "Invalid %ld/%ld for %ld/%ld\n",
+			numerator, denominator,
+			dphy_opts->hs_clk_rate, ref_clk);
+		return -EINVAL;
+	}
+
+	while ((numerator < 16) && (denominator <= 128)) {
+		numerator <<= 1;
+		denominator <<= 1;
+	}
+	/*
+	 * CM ranges between 16 and 255
+	 * CN ranges between 1 and 32
+	 * CO is power of 2: 1, 2, 4, 8
+	 */
+	i = __ffs(denominator);
+	if (i > 3)
+		i = 3;
+	cfg->cn = denominator >> i;
+	cfg->co = 1 << i;
+	cfg->cm = numerator;
+
+	if (cfg->cm < 16 || cfg->cm > 255 ||
+	    cfg->cn < 1 || cfg->cn > 32 ||
+	    cfg->co < 1 || cfg->co > 8) {
+		dev_err(&phy->dev, "Invalid CM/CN/CO values: %u/%u/%u\n",
+			cfg->cm, cfg->cn, cfg->co);
+		dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
+			dphy_opts->hs_clk_rate, ref_clk,
+			numerator, denominator);
+		return -EINVAL;
+	}
+
+	frequency = ref_clk * numerator / (2 * denominator);
+	dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
+		 frequency, dphy_opts->hs_clk_rate, ref_clk,
+		 numerator, denominator);
+
+	/* LP clock period */
+	lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
+	dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
+		dphy_opts->lp_clk_rate, lp_t);
+	/*
+	 *  hs_prepare: in lp clock periods
+	 */
+	if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
+		dev_err(&phy->dev,
+			"hs_prepare (%u) > 2.5 * lp clock period (%lu)",
+			dphy_opts->hs_prepare, lp_t);
+		return -EINVAL;
+	}
+	/* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
+	if (dphy_opts->hs_prepare < lp_t)
+		n = 0;
+	else
+		n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
+	cfg->m_prg_hs_prepare = n;
+
+	/*
+	 * clk_prepare: in lp clock periods
+	 */
+	if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
+		dev_err(&phy->dev,
+			"clk_prepare (%u) > 1.5 * lp clock period (%lu)",
+			dphy_opts->clk_prepare, lp_t);
+		return -EINVAL;
+	}
+	/* 00: lp_t, 01: 1.5 * lp_t */
+	cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 : 0;
+
+	/*
+	 * hs_zero: forumula from NXP BSP
+	 */
+	n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) / 10000;
+	cfg->m_prg_hs_zero = n < 1 ? 1 : n;
+
+	/*
+	 * clk_zero: forumula from NXP BSP
+	 */
+	n = (34 * (dphy_opts->hs_clk_rate / 1000000) - 2500) / 1000;
+	cfg->mc_prg_hs_zero = n < 1 ? 1 : n;
+
+	/*
+	 * clk_trail, hs_trail: forumula from NXP BSP
+	 */
+	n = (103 * (dphy_opts->hs_clk_rate / 1000000) + 10000) / 10000;
+	if (n > 15)
+		n = 15;
+	if (n < 1)
+		n = 1;
+	cfg->m_prg_hs_trail = n;
+	cfg->mc_prg_hs_trail = n;
+
+	/*
+	 * rxhs_settle: formula from NXP BSP
+	 */
+	if (dphy_opts->hs_clk_rate < MBPS(80))
+		cfg->rxhs_settle = 0x0d;
+	else if (dphy_opts->hs_clk_rate < MBPS(90))
+		cfg->rxhs_settle = 0x0c;
+	else if (dphy_opts->hs_clk_rate < MBPS(125))
+		cfg->rxhs_settle = 0x0b;
+	else if (dphy_opts->hs_clk_rate < MBPS(150))
+		cfg->rxhs_settle = 0x0a;
+	else if (dphy_opts->hs_clk_rate < MBPS(225))
+		cfg->rxhs_settle = 0x09;
+	else if (dphy_opts->hs_clk_rate < MBPS(500))
+		cfg->rxhs_settle = 0x08;
+	else
+		cfg->rxhs_settle = 0x07;
+
+	dev_dbg(&phy->dev, "hs_prepare: %u, clk_prepare: %u, "
+		"hs_prepare: %u, clk_prepare: %u, "
+		"hs_trail: %u, clk_trail: %u, "
+		"rxhs_settle: %u\n",
+		cfg->m_prg_hs_prepare, cfg->mc_prg_hs_prepare,
+		cfg->m_prg_hs_zero, cfg->mc_prg_hs_zero,
+		cfg->m_prg_hs_trail, cfg->mc_prg_hs_trail,
+		cfg->rxhs_settle);
+
+	return 0;
+}
+
+static void mixel_phy_set_hs_timings(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	phy_write(phy, priv->cfg.m_prg_hs_prepare, DPHY_M_PRG_HS_PREPARE);
+	phy_write(phy, priv->cfg.mc_prg_hs_prepare, DPHY_MC_PRG_HS_PREPARE);
+	phy_write(phy, priv->cfg.m_prg_hs_zero, DPHY_M_PRG_HS_ZERO);
+	phy_write(phy, priv->cfg.mc_prg_hs_zero, DPHY_MC_PRG_HS_ZERO);
+	phy_write(phy, priv->cfg.m_prg_hs_trail, DPHY_M_PRG_HS_TRAIL);
+	phy_write(phy, priv->cfg.mc_prg_hs_trail, DPHY_MC_PRG_HS_TRAIL);
+	phy_write(phy, priv->cfg.rxhs_settle, priv->devdata->reg_rxhs_settle);
+}
+
+static int mixel_dphy_set_pll_params(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = dev_get_drvdata(phy->dev.parent);
+
+	if (priv->cfg.cm < 16 || priv->cfg.cm > 255 ||
+	    priv->cfg.cn < 1 || priv->cfg.cn > 32 ||
+	    priv->cfg.co < 1 || priv->cfg.co > 8) {
+		dev_err(&phy->dev, "Invalid CM/CN/CO values! (%u/%u/%u)\n",
+			priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+		return -EINVAL;
+	}
+	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
+		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+	phy_write(phy, CM(priv->cfg.cm), DPHY_CM);
+	phy_write(phy, CN(priv->cfg.cn), DPHY_CN);
+	phy_write(phy, CO(priv->cfg.co), DPHY_CO);
+	return 0;
+}
+
+static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	struct mixel_dphy_cfg cfg = { 0 };
+	int ret;
+
+	ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+	if (ret)
+		return ret;
+
+	/* Update the configuration */
+	memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
+
+	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
+		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+
+	phy_write(phy, 0x00, DPHY_LOCK_BYP);
+	phy_write(phy, 0x01, priv->devdata->reg_tx_rcal);
+	phy_write(phy, 0x00, priv->devdata->reg_auto_pd_en);
+	phy_write(phy, 0x02, priv->devdata->reg_rxlprp);
+	phy_write(phy, 0x02, priv->devdata->reg_rxcdrp);
+	phy_write(phy, 0x25, DPHY_TST);
+
+	mixel_phy_set_hs_timings(phy);
+	ret = mixel_dphy_set_pll_params(phy);
+	if (ret < 0)
+		return ret;
+
+	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)
+		return -EINVAL;
+
+	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+}
+
+static int mixel_dphy_power_on(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	u32 locked;
+
+	clk_prepare_enable(priv->phy_ref_clk);
+
+	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
+	phy_write(phy, PWR_ON, DPHY_PD_PLL);
+
+	if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
+				     locked, 10, 1000) < 0) {
+		dev_err(&phy->dev, "Could not get DPHY lock!\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int mixel_dphy_power_off(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	phy_write(phy, PWR_OFF, DPHY_PD_PLL);
+	phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
+
+	clk_disable_unprepare(priv->phy_ref_clk);
+
+	return 0;
+}
+
+static const struct phy_ops mixel_dphy_phy_ops = {
+	.power_on = mixel_dphy_power_on,
+	.power_off = mixel_dphy_power_off,
+	.configure = mixel_dphy_configure,
+	.validate = mixel_dphy_validate,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id mixel_dphy_of_match[] = {
+	{ .compatible = "mixel,imx8mq-mipi-dphy",
+	  .data = &mixel_dphy_devdata[MIXEL_IMX8MQ] },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
+
+static int mixel_dphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct phy_provider *phy_provider;
+	struct mixel_dphy_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	void __iomem *regs;
+
+	if (!np)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->devdata = of_device_get_match_data(&pdev->dev);
+	if (!priv->devdata)
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	regs = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(priv->regs)) {
+		dev_err(dev, "Couldn't map the DPHY registers\n");
+		return PTR_ERR(priv->regs);
+	}
+
+	priv->regs = devm_regmap_init_mmio(&pdev->dev, regs,
+					   &mixel_dphy_regmap_config);
+	if (IS_ERR(priv->regs)) {
+		dev_err(dev, "Couldn't create the DPHY regmap\n");
+		return PTR_ERR(priv->regs);
+	}
+
+	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
+	if (IS_ERR(priv->phy_ref_clk)) {
+		dev_err(dev, "No phy_ref clock found\n");
+		return PTR_ERR(priv->phy_ref_clk);
+	}
+	dev_dbg(dev, "phy_ref clock rate: %lu\n",
+		clk_get_rate(priv->phy_ref_clk));
+
+	dev_set_drvdata(dev, priv);
+
+	phy = devm_phy_create(dev, np, &mixel_dphy_phy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create phy %ld\n", PTR_ERR(phy));
+		return PTR_ERR(phy);
+	}
+	phy_set_drvdata(phy, priv);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static struct platform_driver mixel_dphy_driver = {
+	.probe	= mixel_dphy_probe,
+	.driver = {
+		.name = "mixel-mipi-dphy",
+		.of_match_table	= mixel_dphy_of_match,
+	}
+};
+module_platform_driver(mixel_dphy_driver);
+
+MODULE_AUTHOR("NXP Semiconductor");
+MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
+MODULE_LICENSE("GPL v2");