Hi,
On Mon, Jan 28, 2019 at 01:10:45PM -0200, Fabio Estevam wrote:
> 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.
Seems some of these changed quite a bit between the 4.9 and 4.14 NXP
tree. I'll update things accordingly (and address the other stuff as
well).
-- Guido
>
> > + 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
