Hi Guido,
On Fri, Jan 25, 2019 at 8:15 AM Guido Günther <agx@xxxxxxxxxxx> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
