[PATCH 0/2] RFC: Mixel DPHY support for i.MX8

[PATCH 0/2] RFC: Mixel DPHY support for i.MX8

[Guido Günther]

This adds initial support for the Mixel IP based mipi dphy as found on i.MX8
processors.  It has support for the i.MX8MQ, support for other variants can be
added - once the necessary parts are in - via the provided hooks.
The driver is somewhat based on what's found in NXPs BSP.

Documentation on the DPHY's registers is currently thin in the i.MX8 reference
manuals (even on the i.MX8QXP form 11/18) so most of the values were taken from
existing drivers.

This is based on linux-next as of 2019-01-21 and Maxime Ripard's dphy
configuration helper patches which would need to go in first.

To get DSI working on the imx8MQ there's a driver for the nwl dsi host
controller missing which I intend to looking at too.

Guido Günther (2):
  dt-bindings: phy: Add documentation for mixel dphy
  phy: Add driver for mixel dphy

 .../bindings/phy/mixel,mipi-dsi-phy.txt       |  29 ++
 drivers/phy/Kconfig                           |   7 +
 drivers/phy/Makefile                          |   1 +
 drivers/phy/phy-mixel-mipi-dphy.c             | 449 ++++++++++++++++++
 4 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
 create mode 100644 drivers/phy/phy-mixel-mipi-dphy.c

-- 
2.20.1


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

[PATCH 1/2] dt-bindings: phy: Add documentation for mixel dphy

[Guido Günther]

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../bindings/phy/mixel,mipi-dsi-phy.txt       | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
new file mode 100644
index 000000000000..10323ae8ee37
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
@@ -0,0 +1,29 @@
+Mixel DSI PHY for i.MX8
+
+The Mixel MIPI-DSI PHY IP block is e.g. found on MX8 platforms (along
+the MIPI-DSI IP from Northwest Logic). It represents the physical
+layer for the electrical signals for DSI.
+
+Required properties:
+- compatible: Must be:
+  - "mixel,imx8mq-mipi-dphy"
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+  - "phy_ref": phandle and specifier referring to the DPHY ref clock
+- reg: the register range of the PHY controller
+- #phy-cells: number of cells in PHY, as defined in
+  Documentation/devicetree/bindings/phy/phy-bindings.txt
+  this must be <0>
+
+Optional properties:
+- power-domains: phandle to power domain
+
+Example:
+	mipi_dphy: mipi_dphy@30A0030 {
+		compatible = "mixel,imx8mq-mipi-dphy";
+		clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
+		clock-names = "phy_ref";
+		reg = <0x30A00300 0x100>;
+		power-domains = <&pd_mipi0>;
+		#phy-cells = <0>;
+        };
-- 
2.20.1


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

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

[Guido Günther]

This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
this is an IP core it will likely be found on others in the future. So
instead of adding this to the nwl host driver make it a generic PHY
driver.

The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be
added once the necessary system controller bits are in via
mixel_dpy_ops.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/phy/Kconfig               |   7 +
 drivers/phy/Makefile              |   1 +
 drivers/phy/phy-mixel-mipi-dphy.c | 449 ++++++++++++++++++++++++++++++
 3 files changed, 457 insertions(+)
 create mode 100644 drivers/phy/phy-mixel-mipi-dphy.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 250abe290ca1..9195b5876bcc 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -48,6 +48,13 @@ config PHY_XGENE
 	help
 	  This option enables support for APM X-Gene SoC multi-purpose PHY.
 
+config PHY_MIXEL_MIPI_DPHY
+	bool
+	depends on OF
+	select GENERIC_PHY
+	select GENERIC_PHY_MIPI_DPHY
+	default ARCH_MXC && ARM64
+
 source "drivers/phy/allwinner/Kconfig"
 source "drivers/phy/amlogic/Kconfig"
 source "drivers/phy/broadcom/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 0d9fddc498a6..264f570b67bf 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)		+= mediatek/
 obj-$(CONFIG_ARCH_RENESAS)		+= renesas/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_ARCH_TEGRA)		+= tegra/
+obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-mixel-mipi-dphy.o
 obj-y					+= broadcom/	\
 					   cadence/	\
 					   freescale/	\
diff --git a/drivers/phy/phy-mixel-mipi-dphy.c b/drivers/phy/phy-mixel-mipi-dphy.c
new file mode 100644
index 000000000000..8a43dab79cee
--- /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
+ */
+
+/* #define DEBUG 1 */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+
+/* 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
+
+#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)
+
+/* PHY power on is LOW_ENABLE */
+#define PWR_ON	0
+#define PWR_OFF	1
+
+struct mixel_dphy_cfg {
+	u32 cm;
+	u32 cn;
+	u32 co;
+	unsigned long hs_clk_rate;
+	u8 mc_prg_hs_prepare;
+	u8 m_prg_hs_prepare;
+	u8 mc_prg_hs_zero;
+	u8 m_prg_hs_zero;
+	u8 mc_prg_hs_trail;
+	u8 m_prg_hs_trail;
+};
+
+struct mixel_dphy_priv;
+struct mixel_dphy_ops {
+	int (*probe)(struct mixel_dphy_priv *priv);
+	int (*power_on)(struct phy *phy);
+	int (*power_off)(struct phy *phy);
+};
+
+struct mixel_dphy_priv {
+	struct mixel_dphy_cfg cfg;
+	void __iomem *regs;
+	struct clk *phy_ref_clk;
+	struct mutex lock;
+	const struct mixel_dphy_ops *ops;
+};
+
+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);
+}
+
+static inline void phy_write(struct phy *phy, u32 value, unsigned int reg)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	writel(value, priv->regs + reg);
+}
+
+/* Find a ratio close to the desired one using continued fraction
+   approximation ending either at exact match or maximum allowed
+   nominator, denominator. */
+static void get_best_ratio(unsigned long *pnum, unsigned long *pdenom, unsigned max_n, unsigned max_d)
+{
+	unsigned long a = *pnum;
+	unsigned long b = *pdenom;
+	unsigned long c;
+	unsigned n[] = {0, 1};
+	unsigned d[] = {1, 0};
+	unsigned whole;
+	unsigned i = 1;
+	while (b) {
+		i ^= 1;
+		whole = a / b;
+		n[i] += (n[i ^ 1] * whole);
+		d[i] += (d[i ^ 1] * whole);
+		if ((n[i] > max_n) || (d[i] > max_d)) {
+			i ^= 1;
+			break;
+		}
+		c = a - (b * whole);
+		a = b;
+		b = c;
+	}
+	*pnum = n[i];
+	*pdenom = d[i];
+}
+
+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",
+			numerator, denominator,
+			dphy_opts->hs_clk_rate, ref_clk);
+		return -EINVAL;
+	}
+
+	while ((numerator < 16) && (denominator <= 128)) {
+		numerator <<= 1;
+		denominator <<= 1;
+	}
+	/*
+	 * CM ranges between 16 and 255
+	 * CN ranges between 1 and 32
+	 * CO is power of 2: 1, 2, 4, 8
+	 */
+	i = __ffs(denominator);
+	if (i > 3)
+		i = 3;
+	cfg->cn = denominator >> i;
+	cfg->co = 1 << i;
+	cfg->cm = numerator;
+
+	if (cfg->cm < 16 || cfg->cm > 255 ||
+	    cfg->cn < 1 || cfg->cn > 32 ||
+	    cfg->co < 1 || cfg->co > 8) {
+		dev_err(&phy->dev, "Invalid CM/CN/CO values: %u/%u/%u\n",
+			cfg->cm, cfg->cn, cfg->co);
+		dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
+			dphy_opts->hs_clk_rate, ref_clk,
+			numerator, denominator);
+		return -EINVAL;
+	}
+
+	frequency = ref_clk * numerator / (2 * denominator);
+	dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
+		 frequency, dphy_opts->hs_clk_rate, ref_clk,
+		 numerator, denominator);
+
+	/*
+	 * Section 13.5.11 in the imx8mq reference manual
+	 * IMX8MDQLQRM Rev. 0, 01/2018 does not document
+	 * the values so use the ones found in NXPs BSP.
+	 */
+	cfg->m_prg_hs_prepare = cfg->hs_clk_rate > MBPS(250) ? 0 : 1;
+	cfg->mc_prg_hs_prepare = cfg->hs_clk_rate < MBPS(1000) ? 0 : 1;
+	cfg->m_prg_hs_zero = cfg->hs_clk_rate < MBPS(1000) ? 9 : 10;
+	step = (DATA_RATE_MAX_SPEED - DATA_RATE_MIN_SPEED) / 48;
+	cfg->mc_prg_hs_zero = ((cfg->hs_clk_rate - DATA_RATE_MIN_SPEED) / step) + 1;
+	if (cfg->hs_clk_rate < MBPS(1000)) {
+		cfg->m_prg_hs_trail = 0x05;
+		cfg->mc_prg_hs_trail = 0x05;
+	} else if (cfg->hs_clk_rate < MBPS(1500)) {
+		cfg->m_prg_hs_trail = 0x0C;
+		cfg->mc_prg_hs_trail = 0x0C;
+	} else {
+		cfg->m_prg_hs_trail = 0x0F;
+		cfg->mc_prg_hs_trail = 0x0F;
+	}
+	dev_dbg(&phy->dev, "hs_prepare: %u, clk_prepare: %u, "
+		"hs_prepare: %u, clk_prepare: %u, "
+		"hs_trail: %u, clk_trail: %u\n",
+		cfg->m_prg_hs_prepare, cfg->mc_prg_hs_prepare,
+		cfg->m_prg_hs_zero, cfg->mc_prg_hs_zero,
+		cfg->m_prg_hs_trail, cfg->mc_prg_hs_trail);
+
+	return 0;
+}
+
+static void mixel_phy_set_hs_timings(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	phy_write(phy, priv->cfg.m_prg_hs_prepare, DPHY_M_PRG_HS_PREPARE);
+	phy_write(phy, priv->cfg.mc_prg_hs_prepare, DPHY_MC_PRG_HS_PREPARE);
+	phy_write(phy, priv->cfg.m_prg_hs_zero, DPHY_M_PRG_HS_ZERO);
+	phy_write(phy, priv->cfg.mc_prg_hs_zero, DPHY_MC_PRG_HS_ZERO);
+	phy_write(phy, priv->cfg.m_prg_hs_trail, DPHY_M_PRG_HS_TRAIL);
+	phy_write(phy, priv->cfg.mc_prg_hs_trail, DPHY_MC_PRG_HS_TRAIL);
+}
+
+static int mixel_dphy_set_pll_params(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = dev_get_drvdata(phy->dev.parent);
+
+	if (priv->cfg.cm < 16 || priv->cfg.cm > 255 ||
+	    priv->cfg.cn < 1 || priv->cfg.cn > 32 ||
+	    priv->cfg.co < 1 || priv->cfg.co > 8) {
+		dev_err(&phy->dev, "Invalid CM/CN/CO values! (%u/%u/%u)\n",
+			priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+		return -EINVAL;
+	}
+	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
+		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+	phy_write(phy, CM(priv->cfg.cm), DPHY_CM);
+	phy_write(phy, CN(priv->cfg.cn), DPHY_CN);
+	phy_write(phy, CO(priv->cfg.co), DPHY_CO);
+	return 0;
+}
+
+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;
+		}
+	}
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int mixel_dphy_ref_power_off(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	int ret = 0;
+
+	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;
+}
+
+static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+	struct mixel_dphy_cfg cfg = { 0 };
+	int ret;
+
+	ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+	if (ret)
+		return ret;
+
+	/* Update the configuration */
+	memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
+
+	dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
+		priv->cfg.cm, priv->cfg.cn, priv->cfg.co);
+
+	mutex_lock(&priv->lock);
+
+	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);
+	phy_write(phy, 0x25, DPHY_TST);
+
+	mixel_phy_set_hs_timings(phy);
+	ret = mixel_dphy_set_pll_params(phy);
+	if (ret < 0) {
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int mixel_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
+			       union phy_configure_opts *opts)
+{
+	struct mixel_dphy_cfg cfg = { 0 };
+
+	if (mode != PHY_MODE_MIPI_DPHY)
+		return -EINVAL;
+
+	return mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+}
+
+static int mixel_dphy_power_on(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	if (priv->ops->power_on)
+		return priv->ops->power_on(phy);
+	return 0;
+}
+
+static int mixel_dphy_power_off(struct phy *phy)
+{
+	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
+
+	if (priv->ops->power_off)
+		return priv->ops->power_off(phy);
+	return 0;
+}
+
+/*
+ * This is the reference implementation of DPHY hooks. Specific integration of
+ * this IP may have to re-implement some of them depending on how they decided
+ * to wire things in the SoC.
+ */
+static const struct mixel_dphy_ops mixel_dphy_ref_ops = {
+	.power_on = mixel_dphy_ref_power_on,
+	.power_off = mixel_dphy_ref_power_off,
+};
+
+static const struct phy_ops mixel_dphy_ops = {
+	.power_on = mixel_dphy_power_on,
+	.power_off = mixel_dphy_power_off,
+	.configure = mixel_dphy_configure,
+	.validate = mixel_dphy_validate,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id mixel_dphy_of_match[] = {
+	{ .compatible = "mixel,imx8mq-mipi-dphy", .data = &mixel_dphy_ref_ops },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
+
+static int mixel_dphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct phy_provider *phy_provider;
+	struct mixel_dphy_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	int ret;
+
+	if (!np)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ops = of_device_get_match_data(&pdev->dev);
+	if (!priv->ops)
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	priv->regs = devm_ioremap(dev, res->start, SZ_256);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
+	if (IS_ERR(priv->phy_ref_clk)) {
+		dev_err(dev, "No phy_ref clock found");
+		return PTR_ERR(priv->phy_ref_clk);
+	}
+	dev_dbg(dev, "phy_ref clock rate: %lu", clk_get_rate(priv->phy_ref_clk));
+
+	mutex_init(&priv->lock);
+	dev_set_drvdata(dev, priv);
+
+	if (priv->ops->probe) {
+		ret = priv->ops->probe(priv);
+		if (ret)
+			return ret;
+	}
+
+	phy = devm_phy_create(dev, np, &mixel_dphy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create phy %ld\n", PTR_ERR(phy));
+		return PTR_ERR(phy);
+	}
+	phy_set_drvdata(phy, priv);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static struct platform_driver mixel_dphy_driver = {
+	.probe	= mixel_dphy_probe,
+	.driver = {
+		.name = "mixel-mipi-dphy",
+		.of_match_table	= mixel_dphy_of_match,
+	}
+};
+module_platform_driver(mixel_dphy_driver);
+
+MODULE_AUTHOR("NXP Semiconductor");
+MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1


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

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

[Sam Ravnborg]

Hi Guido.

Patch looks good but a few comments below.

	Sam

On Fri, Jan 25, 2019 at 11:14:46AM +0100, Guido Günther wrote:
> This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
> this is an IP core it will likely be found on others in the future. So
> instead of adding this to the nwl host driver make it a generic PHY
> driver.
> 
> The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be
> added once the necessary system controller bits are in via
> mixel_dpy_ops.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  drivers/phy/Kconfig               |   7 +
>  drivers/phy/Makefile              |   1 +
>  drivers/phy/phy-mixel-mipi-dphy.c | 449 ++++++++++++++++++++++++++++++
>  3 files changed, 457 insertions(+)
>  create mode 100644 drivers/phy/phy-mixel-mipi-dphy.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 250abe290ca1..9195b5876bcc 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -48,6 +48,13 @@ config PHY_XGENE
>  	help
>  	  This option enables support for APM X-Gene SoC multi-purpose PHY.
>  
> +config PHY_MIXEL_MIPI_DPHY
> +	bool
> +	depends on OF
> +	select GENERIC_PHY
> +	select GENERIC_PHY_MIPI_DPHY
> +	default ARCH_MXC && ARM64

Is it correct that driver is mandatory if ARCH_MXC is y?
There is no prompt to allow the user to select it.
Or in other words - will all i.MX8 user need it?

> +
>  source "drivers/phy/allwinner/Kconfig"
>  source "drivers/phy/amlogic/Kconfig"
>  source "drivers/phy/broadcom/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 0d9fddc498a6..264f570b67bf 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)		+= mediatek/
>  obj-$(CONFIG_ARCH_RENESAS)		+= renesas/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_ARCH_TEGRA)		+= tegra/
> +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-mixel-mipi-dphy.o
>  obj-y					+= broadcom/	\
>  					   cadence/	\
>  					   freescale/	\
> diff --git a/drivers/phy/phy-mixel-mipi-dphy.c b/drivers/phy/phy-mixel-mipi-dphy.c
> new file mode 100644
> index 000000000000..8a43dab79cee
> --- /dev/null
> +++ b/drivers/phy/phy-mixel-mipi-dphy.c

There is already a PHY named phy-fsl-imx8mq-usb, located in the
freescale subdirectory.
Why locate another imx8 PHY in the top level directory with
another naming convention?

> @@ -0,0 +1,449 @@
> +/*
> + * Copyright 2017 NXP
> + * Copyright 2019 Purism SPC
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
SPDX-License-Identifier goes in at first line with //.
It is documented somewhere.
Also, did you double check that GPL 2.0 is correct?

> +
> +/* #define DEBUG 1 */
There is no reference to DEBUG in this file - delete?

> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +
> +/* 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
> +
> +#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)
> +
> +/* PHY power on is LOW_ENABLE */
> +#define PWR_ON	0
> +#define PWR_OFF	1
> +
> +struct mixel_dphy_cfg {
> +	u32 cm;
> +	u32 cn;
> +	u32 co;
> +	unsigned long hs_clk_rate;
> +	u8 mc_prg_hs_prepare;
> +	u8 m_prg_hs_prepare;
> +	u8 mc_prg_hs_zero;
> +	u8 m_prg_hs_zero;
> +	u8 mc_prg_hs_trail;
> +	u8 m_prg_hs_trail;
> +};

For the naive reader it would be helpful to spell out the names in a comment.
As I assume the names comes from the data sheet the short names are OK - but
let others know the purpose.

> +
> +struct mixel_dphy_priv;
> +struct mixel_dphy_ops {
> +	int (*probe)(struct mixel_dphy_priv *priv);
> +	int (*power_on)(struct phy *phy);
> +	int (*power_off)(struct phy *phy);
> +};
Consider same argument for all three ops, less suprises.
But then probe() is called before we have a phy, so this may be
the best option.
> +
> +struct mixel_dphy_priv {
> +	struct mixel_dphy_cfg cfg;
> +	void __iomem *regs;
> +	struct clk *phy_ref_clk;
> +	struct mutex lock;
> +	const struct mixel_dphy_ops *ops;
> +};
Document what the lock protects, or find a better name for the lock to document it

> +
> +/* Find a ratio close to the desired one using continued fraction
> +   approximation ending either at exact match or maximum allowed
> +   nominator, denominator. */

Use kernel style comments
/*
 * Bla bla
 * more bla
 */

> +static void get_best_ratio(unsigned long *pnum, unsigned long *pdenom, unsigned max_n, unsigned max_d)
Wrap line to stay below 80 chars.
Use checkpatch to help you sport things like this.

> +{
> +	unsigned long a = *pnum;
> +	unsigned long b = *pdenom;
> +	unsigned long c;
> +	unsigned n[] = {0, 1};
> +	unsigned d[] = {1, 0};
> +	unsigned whole;
> +	unsigned i = 1;
> +	while (b) {
Add empty line after last local variable.

> +		i ^= 1;
> +		whole = a / b;
> +		n[i] += (n[i ^ 1] * whole);
> +		d[i] += (d[i ^ 1] * whole);
> +		if ((n[i] > max_n) || (d[i] > max_d)) {
> +			i ^= 1;
> +			break;
> +		}
> +		c = a - (b * whole);
> +		a = b;
> +		b = c;
> +	}
> +	*pnum = n[i];
> +	*pdenom = d[i];
> +}
> +
> +static int mixel_dphy_config_from_opts(struct phy *phy,
> +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> +	       struct mixel_dphy_cfg *cfg)
> +{
Align extra paratmers below the first parameter using tabs and add necessary
spaces.

> +	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;
> +
> +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;
> +		}
USe goto to have a single exit path where you do mutex_unlock()

> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +
> +	mutex_lock(&priv->lock);
> +
> +	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);
> +	phy_write(phy, 0x25, DPHY_TST);
> +
> +	mixel_phy_set_hs_timings(phy);
> +	ret = mixel_dphy_set_pll_params(phy);
> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
USe goto to have a single exit path where you do mutex_unlock()
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +
> +/*
> + * This is the reference implementation of DPHY hooks. Specific integration of
> + * this IP may have to re-implement some of them depending on how they decided
> + * to wire things in the SoC.
> + */
> +static const struct mixel_dphy_ops mixel_dphy_ref_ops = {
> +	.power_on = mixel_dphy_ref_power_on,
> +	.power_off = mixel_dphy_ref_power_off,
> +};
> +
> +static const struct phy_ops mixel_dphy_ops = {
> +	.power_on = mixel_dphy_power_on,
> +	.power_off = mixel_dphy_power_off,
> +	.configure = mixel_dphy_configure,
> +	.validate = mixel_dphy_validate,
> +	.owner = THIS_MODULE,
> +};
This is confusing.
We have struct mixel_dphy_ops => mixel_dphy_ref_ops
And then struct phy_ops => mixel_dphy_ops

So reading this there are to uses of mixel_dphy_ops,
one is a struct, and another is an instance of another type.
Try to find a niming scheme that is less confusing.


> +
> +static const struct of_device_id mixel_dphy_of_match[] = {
> +	{ .compatible = "mixel,imx8mq-mipi-dphy", .data = &mixel_dphy_ref_ops },
> +	{ /* sentinel */ },
> +};
Multi-line to keep line shorter?

> +MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> +
> +static int mixel_dphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct phy_provider *phy_provider;
> +	struct mixel_dphy_priv *priv;
> +	struct resource *res;
> +	struct phy *phy;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->ops = of_device_get_match_data(&pdev->dev);
> +	if (!priv->ops)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	priv->regs = devm_ioremap(dev, res->start, SZ_256);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);
> +
> +	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> +	if (IS_ERR(priv->phy_ref_clk)) {
> +		dev_err(dev, "No phy_ref clock found");
> +		return PTR_ERR(priv->phy_ref_clk);
> +	}
> +	dev_dbg(dev, "phy_ref clock rate: %lu", clk_get_rate(priv->phy_ref_clk));
> +
> +	mutex_init(&priv->lock);
> +	dev_set_drvdata(dev, priv);
> +
> +	if (priv->ops->probe) {
> +		ret = priv->ops->probe(priv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	phy = devm_phy_create(dev, np, &mixel_dphy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "Failed to create phy %ld\n", PTR_ERR(phy));
> +		return PTR_ERR(phy);
> +	}
> +	phy_set_drvdata(phy, priv);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver mixel_dphy_driver = {
> +	.probe	= mixel_dphy_probe,
> +	.driver = {
> +		.name = "mixel-mipi-dphy",
> +		.of_match_table	= mixel_dphy_of_match,
> +	}
> +};
> +module_platform_driver(mixel_dphy_driver);
> +
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [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@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.

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

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

[Guido Günther]

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

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

[Guido Günther]

Hi Sam,
On Fri, Jan 25, 2019 at 05:53:55PM +0100, Sam Ravnborg wrote:
[..snip..]
> > +struct mixel_dphy_cfg {
> > +	u32 cm;
> > +	u32 cn;
> > +	u32 co;
> > +	unsigned long hs_clk_rate;
> > +	u8 mc_prg_hs_prepare;
> > +	u8 m_prg_hs_prepare;
> > +	u8 mc_prg_hs_zero;
> > +	u8 m_prg_hs_zero;
> > +	u8 mc_prg_hs_trail;
> > +	u8 m_prg_hs_trail;
> > +};
> 
> For the naive reader it would be helpful to spell out the names in a comment.
> As I assume the names comes from the data sheet the short names are OK - but
> let others know the purpose.

These are actual register names so I added comment just saying that.

[..snip..]
> > +static int mixel_dphy_config_from_opts(struct phy *phy,
> > +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> > +	       struct mixel_dphy_cfg *cfg)
> > +{
> Align extra paratmers below the first parameter using tabs and add necessary
> spaces.

Due to the long phy_configure_opts_mipi_dpy this whole hit the 80 char
limit so I left it like that to make checkpatch happy.

> 
> > +	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;
> > +
> > +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;
> > +		}
> USe goto to have a single exit path where you do mutex_unlock()

Using regmap I could drop the lock entirely.

[..snip..]
> > +static const struct mixel_dphy_ops mixel_dphy_ref_ops = {
> > +	.power_on = mixel_dphy_ref_power_on,
> > +	.power_off = mixel_dphy_ref_power_off,
> > +};
> > +
> > +static const struct phy_ops mixel_dphy_ops = {
> > +	.power_on = mixel_dphy_power_on,
> > +	.power_off = mixel_dphy_power_off,
> > +	.configure = mixel_dphy_configure,
> > +	.validate = mixel_dphy_validate,
> > +	.owner = THIS_MODULE,
> > +};
> This is confusing.
> We have struct mixel_dphy_ops => mixel_dphy_ref_ops
> And then struct phy_ops => mixel_dphy_ops
> 
> So reading this there are to uses of mixel_dphy_ops,
> one is a struct, and another is an instance of another type.
> Try to find a niming scheme that is less confusing.

Yeah, that's true. I found in another driver other imx8 variants have
different register offsets so I went for register offsets in devdata
rather than function pointer table which make this ambiguity go away too.
I hope I have tackled all your other comments.

Thanks!
 -- Guido

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