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

["Guido Günther" <agx@sigxcpu.org>]

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@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.

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.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel