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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
