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 <agx@xxxxxxxxxxx>
> > > ---
> > > 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");
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
