[PATCH 2/2] phy: Add driver for mixel dphy

[Fabio Estevam]

Hi Guido,

On Fri, Jan 25, 2019 at 8:15 AM Guido Günther <agx at sigxcpu.org> wrote:

> +config PHY_MIXEL_MIPI_DPHY
> +       bool
> +       depends on OF
> +       select GENERIC_PHY
> +       select GENERIC_PHY_MIPI_DPHY
> +       default ARCH_MXC && ARM64

No need to force this selection for all i.MX8M boards, as not everyone
is interested in using Mixel DSI.

> --- /dev/null
> +++ b/drivers/phy/phy-mixel-mipi-dphy.c
> @@ -0,0 +1,449 @@
> +/*
> + * Copyright 2017 NXP
> + * Copyright 2019 Purism SPC
> + *
> + * SPDX-License-Identifier: GPL-2.0

SPDX line should be in the first line and start with //

> + */
> +
> +/* #define DEBUG 1 */

Please remove it.

> +/* 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_TX_RCAL                   0x38
> +#define DPHY_AUTO_PD_EN                        0x3c
> +#define DPHY_RXLPRP                    0x40
> +#define DPHY_RXCDRP                    0x44

In the NXP vendor tree we have these additional offsets for imx8m that
are missing here:

.reg_rxhs_settle = 0x48,
.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)
> +
> +static inline u32 phy_read(struct phy *phy, unsigned int reg)
> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +
> +       return readl(priv->regs + reg);

Maybe it's worth using regmap here. It makes it very easy to dump all
the MIXEL registers at once.

> +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);
> +       int i;
> +       unsigned long numerator, denominator, frequency;
> +       unsigned step;
> +
> +       if (dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED ||
> +           dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED)
> +               return -EINVAL;
> +       cfg->hs_clk_rate = dphy_opts->hs_clk_rate;
> +
> +       numerator = dphy_opts->hs_clk_rate;
> +       denominator = ref_clk;
> +       get_best_ratio(&numerator, &denominator, 255, 256);
> +       if (!numerator || !denominator) {
> +               dev_dbg(&phy->dev, "Invalid %ld/%ld for %ld/%ld\n",

dev_err should be more appropriate here.

> +static int mixel_dphy_ref_power_on(struct phy *phy)
> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +       u32 lock, timeout;
> +       int ret = 0;
> +
> +       mutex_lock(&priv->lock);
> +       clk_prepare_enable(priv->phy_ref_clk);
> +
> +       phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> +       phy_write(phy, PWR_ON, DPHY_PD_PLL);
> +
> +       timeout = 100;
> +       while (!(lock = phy_read(phy, DPHY_LOCK))) {
> +               udelay(10);
> +               if (--timeout == 0) {
> +                       dev_err(&phy->dev, "Could not get DPHY lock!\n");
> +                       mutex_unlock(&priv->lock);
> +                       return -EINVAL;

Could you use readl_poll_timeout() instead?

> +static int mixel_dphy_ref_power_off(struct phy *phy)
> +{
> +       struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +       int ret = 0;

This variable is not needed.

> +
> +       mutex_lock(&priv->lock);
> +
> +       phy_write(phy, PWR_OFF, DPHY_PD_PLL);
> +       phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
> +
> +       clk_disable_unprepare(priv->phy_ref_clk);
> +       mutex_unlock(&priv->lock);
> +
> +       return ret;

and you could simply do a 'return 0' instead.

> +       phy_write(phy, 0x00, DPHY_LOCK_BYP);
> +       phy_write(phy, 0x01, DPHY_TX_RCAL);
> +       phy_write(phy, 0x00, DPHY_AUTO_PD_EN);
> +       phy_write(phy, 0x01, DPHY_RXLPRP);
> +       phy_write(phy, 0x01, DPHY_RXCDRP);

In the NXP vendor code the value 2 is written to this register:

phy_write(phy, 0x02, priv->plat_data->reg_rxcdrp);

It would be good to dump all Mixel DSI PHY registers in the NXP vendor
and in mainline and make sure they match.

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

No need to hardcode SZ_256 here and the size should come from the device tree.

You could also use devm_ioremap_resource() instead.