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

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.

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