Re: [PATCH 04/10] phy: dphy: Add configuration helpers

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Wednesday, 5 September 2018 12:16:35 EEST Maxime Ripard wrote:
> The MIPI D-PHY spec defines default values and boundaries for most of the
> parameters it defines. Introduce helpers to help drivers get meaningful
> values based on their current parameters, and validate the boundaries of
> these parameters if needed.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> ---
>  drivers/phy/Kconfig               |   8 ++-
>  drivers/phy/Makefile              |   1 +-
>  drivers/phy/phy-core-mipi-dphy.c  | 160 +++++++++++++++++++++++++++++++-
>  include/linux/phy/phy-mipi-dphy.h |   6 +-
>  4 files changed, 175 insertions(+)
>  create mode 100644 drivers/phy/phy-core-mipi-dphy.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 5c8d452e35e2..06bd22bd1f4a 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -15,6 +15,14 @@ config GENERIC_PHY
>  	  phy users can obtain reference to the PHY. All the users of this
>  	  framework should select this config.
> 
> +config GENERIC_PHY_MIPI_DPHY
> +	bool "MIPI D-PHY support"
> +	help
> +	  Generic MIPI D-PHY support.
> +
> +	  Provides a number of helpers a core functions for MIPI D-PHY
> +	  drivers to us.

Do we really need to make this user-selectable ?

>  config PHY_LPC18XX_USB_OTG
>  	tristate "NXP LPC18xx/43xx SoC USB OTG PHY driver"
>  	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 84e3bd9c5665..71c29d2b9af7 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -4,6 +4,7 @@
>  #
> 
>  obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
> +obj-$(CONFIG_GENERIC_PHY_MIPI_DPHY)	+= phy-core-mipi-dphy.o
>  obj-$(CONFIG_PHY_LPC18XX_USB_OTG)	+= phy-lpc18xx-usb-otg.o
>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
>  obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
> diff --git a/drivers/phy/phy-core-mipi-dphy.c
> b/drivers/phy/phy-core-mipi-dphy.c new file mode 100644
> index 000000000000..6c1ddc7734a2
> --- /dev/null
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2013 NVIDIA Corporation
> + * Copyright (C) 2018 Cadence Design Systems Inc.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/time64.h>
> +
> +#include <linux/phy/phy.h>
> +#include <linux/phy/phy-mipi-dphy.h>
> +
> +/*
> + * Default D-PHY timings based on MIPI D-PHY specification. Derived from
> the
> + * valid ranges specified in Section 6.9, Table 14, Page 40 of the D-PHY
> + * specification (v1.2) with minor adjustments.

Could you list those adjustments ?

> + */
> +int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
> +				     unsigned int bpp,
> +				     unsigned int lanes,
> +				     struct phy_configure_opts_mipi_dphy *cfg)
> +{
> +	unsigned long hs_clk_rate;
> +	unsigned long ui;
> +
> +	if (!cfg)
> +		return -EINVAL;

Should we really expect cfg to be NULL ?

> +	hs_clk_rate = pixel_clock * bpp / lanes;
> +	ui = DIV_ROUND_UP(NSEC_PER_SEC, hs_clk_rate);
> +
> +	cfg->clk_miss = 0;
> +	cfg->clk_post = 70 + 52 * ui;
> +	cfg->clk_pre = 8;
> +	cfg->clk_prepare = 65;
> +	cfg->clk_settle = 95;
> +	cfg->clk_term_en = 0;
> +	cfg->clk_trail = 80;
> +	cfg->clk_zero = 260;
> +	cfg->d_term_en = 0;
> +	cfg->eot = 0;
> +	cfg->hs_exit = 120;
> +	cfg->hs_prepare = 65 + 5 * ui;
> +	cfg->hs_zero = 145 + 5 * ui;
> +	cfg->hs_settle = 85 + 6 * ui;
> +	cfg->hs_skip = 40;
> +
> +	/*
> +	 * The MIPI D-PHY specification (Section 6.9, v1.2, Table 14, Page 40)
> +	 * contains this formula as:
> +	 *
> +	 *     T_HS-TRAIL = max(n * 8 * ui, 60 + n * 4 * ui)
> +	 *
> +	 * where n = 1 for forward-direction HS mode and n = 4 for reverse-
> +	 * direction HS mode. There's only one setting and this function does
> +	 * not parameterize on anything other that ui, so this code will
> +	 * assumes that reverse-direction HS mode is supported and uses n = 4.
> +	 */
> +	cfg->hs_trail = max(4 * 8 * ui, 60 + 4 * 4 * ui);
> +
> +	cfg->init = 100000;
> +	cfg->lpx = 60;
> +	cfg->ta_get = 5 * cfg->lpx;
> +	cfg->ta_go = 4 * cfg->lpx;
> +	cfg->ta_sure = 2 * cfg->lpx;
> +	cfg->wakeup = 1000000;
> +
> +	cfg->hs_clk_rate = hs_clk_rate;
> +	cfg->lanes = lanes;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_mipi_dphy_get_default_config);
> +
> +/*
> + * Validate D-PHY configuration according to MIPI D-PHY specification
> + * (v1.2, Section Section 6.9 "Global Operation Timing Parameters").
> + */
> +int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
> +{
> +	unsigned long ui;
> +
> +	if (!cfg)
> +		return -EINVAL;

Same here.

> +	ui = DIV_ROUND_UP(NSEC_PER_SEC, cfg->hs_clk_rate);
> +
> +	if (cfg->clk_miss > 60)
> +		return -EINVAL;
> +
> +	if (cfg->clk_post < (60 + 52 * ui))
> +		return -EINVAL;
> +
> +	if (cfg->clk_pre < 8)
> +		return -EINVAL;
> +
> +	if (cfg->clk_prepare < 38 || cfg->clk_prepare > 95)
> +		return -EINVAL;
> +
> +	if (cfg->clk_settle < 95 || cfg->clk_settle > 300)
> +		return -EINVAL;
> +
> +	if (cfg->clk_term_en > 38)
> +		return -EINVAL;
> +
> +	if (cfg->clk_trail < 60)
> +		return -EINVAL;
> +
> +	if (cfg->clk_prepare + cfg->clk_zero < 300)
> +		return -EINVAL;
> +
> +	if (cfg->d_term_en > 35 + 4 * ui)
> +		return -EINVAL;
> +
> +	if (cfg->eot > 105 + 12 * ui)
> +		return -EINVAL;
> +
> +	if (cfg->hs_exit < 100)
> +		return -EINVAL;
> +
> +	if (cfg->hs_prepare < 40 + 4 * ui ||
> +	    cfg->hs_prepare > 85 + 6 * ui)
> +		return -EINVAL;
> +
> +	if (cfg->hs_prepare + cfg->hs_zero < 145 + 10 * ui)
> +		return -EINVAL;
> +
> +	if ((cfg->hs_settle < 85 + 6 * ui) ||
> +	    (cfg->hs_settle > 145 + 10 * ui))
> +		return -EINVAL;
> +
> +	if (cfg->hs_skip < 40 || cfg->hs_skip > 55 + 4 * ui)
> +		return -EINVAL;
> +
> +	if (cfg->hs_trail < max(8 * ui, 60 + 4 * ui))
> +		return -EINVAL;
> +
> +	if (cfg->init < 100000)
> +		return -EINVAL;
> +
> +	if (cfg->lpx < 50)
> +		return -EINVAL;
> +
> +	if (cfg->ta_get != 5 * cfg->lpx)
> +		return -EINVAL;
> +
> +	if (cfg->ta_go != 4 * cfg->lpx)
> +		return -EINVAL;
> +
> +	if (cfg->ta_sure < cfg->lpx || cfg->ta_sure > 2 * cfg->lpx)
> +		return -EINVAL;
> +
> +	if (cfg->wakeup < 1000000)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_mipi_dphy_config_validate);
> diff --git a/include/linux/phy/phy-mipi-dphy.h
> b/include/linux/phy/phy-mipi-dphy.h index 792724145290..7656d057198f 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -238,4 +238,10 @@ struct phy_configure_opts_mipi_dphy {
>  /* TODO: Add other modes (burst, commands, etc) */
>  #define MIPI_DPHY_MODE_VIDEO_SYNC_PULSE		BIT(0)
> 
> +int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
> +				     unsigned int bpp,
> +				     unsigned int lanes,
> +				     struct phy_configure_opts_mipi_dphy *cfg);
> +int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy
> *cfg);
> +
>  #endif /* __PHY_MIPI_DPHY_H_ */


-- 
Regards,

Laurent Pinchart