Date:   Mon, 9 Jul 2018 10:49:50 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
CC:     <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC

Hi,

On Friday 29 June 2018 02:08 PM, Kunihiko Hayashi wrote:
> Add a driver for PHY interface built into USB3 controller
> implemented in UniPhier SoCs.
> This driver supports High-Speed PHY and Super-Speed PHY.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
> Signed-off-by: Motoya Tanigawa <tanigawa.motoya@...ionext.com>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...aro.org>
> ---
>  drivers/phy/Kconfig                         |   1 +
>  drivers/phy/Makefile                        |   1 +
>  drivers/phy/socionext/Kconfig               |  12 +
>  drivers/phy/socionext/Makefile              |   6 +
>  drivers/phy/socionext/phy-uniphier-usb3hs.c | 422 ++++++++++++++++++++++++++++
>  drivers/phy/socionext/phy-uniphier-usb3ss.c | 369 ++++++++++++++++++++++++
>  6 files changed, 811 insertions(+)
>  create mode 100644 drivers/phy/socionext/Kconfig
>  create mode 100644 drivers/phy/socionext/Makefile
>  create mode 100644 drivers/phy/socionext/phy-uniphier-usb3hs.c
>  create mode 100644 drivers/phy/socionext/phy-uniphier-usb3ss.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 5c8d452..b752589 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -53,6 +53,7 @@ source "drivers/phy/ralink/Kconfig"
>  source "drivers/phy/renesas/Kconfig"
>  source "drivers/phy/rockchip/Kconfig"
>  source "drivers/phy/samsung/Kconfig"
> +source "drivers/phy/socionext/Kconfig"
>  source "drivers/phy/st/Kconfig"
>  source "drivers/phy/tegra/Kconfig"
>  source "drivers/phy/ti/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 84e3bd9..5539cde 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -21,5 +21,6 @@ obj-y					+= broadcom/	\
>  					   qualcomm/	\
>  					   ralink/	\
>  					   samsung/	\
> +					   socionext/	\
>  					   st/		\
>  					   ti/
> diff --git a/drivers/phy/socionext/Kconfig b/drivers/phy/socionext/Kconfig
> new file mode 100644
> index 0000000..4a172fc
> --- /dev/null
> +++ b/drivers/phy/socionext/Kconfig
> @@ -0,0 +1,12 @@
> +#
> +# PHY drivers for Socionext platforms.
> +#
> +
> +config PHY_UNIPHIER_USB3
> +	tristate "UniPhier USB3 PHY driver"
> +	depends on ARCH_UNIPHIER || COMPILE_TEST
> +	depends on OF && HAS_IOMEM
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support USB PHY implemented in USB3 controller
> +	  on UniPhier SoCs. This controller supports USB3.0 and lower speed.
> diff --git a/drivers/phy/socionext/Makefile b/drivers/phy/socionext/Makefile
> new file mode 100644
> index 0000000..dfa5cec
> --- /dev/null
> +++ b/drivers/phy/socionext/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-identifier: GPL-2.0
> +#
> +# Makefile for the phy drivers.
> +#
> +
> +obj-$(CONFIG_PHY_UNIPHIER_USB3)	+= phy-uniphier-usb3hs.o phy-uniphier-usb3ss.o
> diff --git a/drivers/phy/socionext/phy-uniphier-usb3hs.c b/drivers/phy/socionext/phy-uniphier-usb3hs.c
> new file mode 100644
> index 0000000..cea8983
> --- /dev/null
> +++ b/drivers/phy/socionext/phy-uniphier-usb3hs.c
> @@ -0,0 +1,422 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * phy-uniphier-usb3hs.c - HS-PHY driver for Socionext UniPhier USB3 controller
> + * Copyright 2015-2018 Socionext Inc.
> + * Author:
> + *	Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
> + * Contributors:
> + *      Motoya Tanigawa <tanigawa.motoya@...ionext.com>
> + *      Masami Hiramatsu <masami.hiramatsu@...aro.org>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define HSPHY_CFG0		0x0
> +#define HSPHY_CFG0_HS_I_MASK	GENMASK(31, 28)
> +#define HSPHY_CFG0_HSDISC_MASK	GENMASK(27, 26)
> +#define HSPHY_CFG0_SWING_MASK	GENMASK(17, 16)
> +#define HSPHY_CFG0_SEL_T_MASK	GENMASK(15, 12)
> +#define HSPHY_CFG0_RTERM_MASK	GENMASK(7, 6)
> +#define HSPHY_CFG0_TRIMMASK	(HSPHY_CFG0_HS_I_MASK \
> +				 | HSPHY_CFG0_SEL_T_MASK \
> +				 | HSPHY_CFG0_RTERM_MASK)
> +
> +#define HSPHY_CFG1		0x4
> +#define HSPHY_CFG1_DAT_EN	BIT(29)
> +#define HSPHY_CFG1_ADR_EN	BIT(28)
> +#define HSPHY_CFG1_ADR_MASK	GENMASK(27, 16)
> +#define HSPHY_CFG1_DAT_MASK	GENMASK(23, 16)
> +
> +#define MAX_CLKS	3
> +#define MAX_RSTS	2
> +#define MAX_PHY_PARAMS	1
> +
> +struct uniphier_u3hsphy_param {
> +	u32 addr;
> +	u32 mask;
> +	u32 val;
> +};

I'd like to avoid configure the PHY this way, since it's impossible to know
which register is being configured.
> +
> +struct uniphier_u3hsphy_trim_param {
> +	unsigned int rterm;
> +	unsigned int sel_t;
> +	unsigned int hs_i;
> +};
> +
> +#define trim_param_is_valid(p)	((p)->rterm || (p)->sel_t || (p)->hs_i)
> +
> +struct uniphier_u3hsphy_priv {
> +	struct device *dev;
> +	void __iomem *base;
> +	int nclks;
> +	struct clk *clk[MAX_CLKS], *clk_phy, *clk_phy_ext;
> +	int nrsts;
> +	struct reset_control *rst[MAX_RSTS], *rst_phy;
> +	const struct uniphier_u3hsphy_soc_data *data;
> +};
> +
> +struct uniphier_u3hsphy_soc_data {
> +	const char *clock_names[MAX_CLKS];
> +	const char *reset_names[MAX_RSTS];
> +	int nparams;
> +	const struct uniphier_u3hsphy_param param[MAX_PHY_PARAMS];
> +	u32 config0;
> +	u32 config1;
> +	void (*trim_func)(struct uniphier_u3hsphy_priv *priv, u32 *pconfig,
> +			  struct uniphier_u3hsphy_trim_param *pt);
> +};
> +
> +static void uniphier_u3hsphy_trim_ld20(struct uniphier_u3hsphy_priv *priv,
> +				       u32 *pconfig,
> +				       struct uniphier_u3hsphy_trim_param *pt)
> +{
> +	*pconfig &= ~HSPHY_CFG0_RTERM_MASK;
> +	*pconfig |= FIELD_PREP(HSPHY_CFG0_RTERM_MASK, pt->rterm);
> +
> +	*pconfig &= ~HSPHY_CFG0_SEL_T_MASK;
> +	*pconfig |= FIELD_PREP(HSPHY_CFG0_SEL_T_MASK, pt->sel_t);
> +
> +	*pconfig &= ~HSPHY_CFG0_HS_I_MASK;
> +	*pconfig |= FIELD_PREP(HSPHY_CFG0_HS_I_MASK,  pt->hs_i);
> +}
> +
> +static int uniphier_u3hsphy_get_nvparam(struct uniphier_u3hsphy_priv *priv,
> +					const char *name, unsigned int *val)
> +{
> +	struct nvmem_cell *cell;
> +	u8 *buf;
> +
> +	cell = devm_nvmem_cell_get(priv->dev, name);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	buf = nvmem_cell_read(cell, NULL);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	*val = *buf;
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
> +static int uniphier_u3hsphy_get_nvparams(struct uniphier_u3hsphy_priv *priv,
> +					 struct uniphier_u3hsphy_trim_param *pt)
> +{
> +	int ret;
> +
> +	ret = uniphier_u3hsphy_get_nvparam(priv, "rterm", &pt->rterm);
> +	if (ret)
> +		return ret;
> +
> +	ret = uniphier_u3hsphy_get_nvparam(priv, "sel_t", &pt->sel_t);
> +	if (ret)
> +		return ret;
> +
> +	ret = uniphier_u3hsphy_get_nvparam(priv, "hs_i", &pt->hs_i);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int uniphier_u3hsphy_update_config(struct uniphier_u3hsphy_priv *priv,
> +					  u32 *pconfig)
> +{
> +	struct uniphier_u3hsphy_trim_param trim;
> +	int ret, trimmed = 0;
> +
> +	if (priv->data->trim_func) {
> +		ret = uniphier_u3hsphy_get_nvparams(priv, &trim);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		/*
> +		 * call trim_func only when trimming parameters that aren't
> +		 * all-zero can be acquired. All-zero parameters mean nothing
> +		 * has been written to nvmem.
> +		 */
> +		if (!ret && trim_param_is_valid(&trim)) {
> +			priv->data->trim_func(priv, pconfig, &trim);
> +			trimmed = 1;
> +		} else {
> +			dev_dbg(priv->dev, "can't get parameter from nvmem\n");
> +		}
> +	}
> +
> +	/* use default parameters without trimming values */
> +	if (!trimmed) {
> +		*pconfig &= ~HSPHY_CFG0_HSDISC_MASK;
> +		*pconfig |= FIELD_PREP(HSPHY_CFG0_HSDISC_MASK, 3);
> +	}
> +
> +	return 0;
> +}
> +
> +static void uniphier_u3hsphy_set_param(struct uniphier_u3hsphy_priv *priv,
> +				       const struct uniphier_u3hsphy_param *p)
> +{
> +	u32 val;
> +
> +	val = readl(priv->base + HSPHY_CFG1);
> +	val &= ~HSPHY_CFG1_ADR_MASK;
> +	val |= FIELD_PREP(HSPHY_CFG1_ADR_MASK, p->addr) | HSPHY_CFG1_ADR_EN;
> +	writel(val, priv->base + HSPHY_CFG1);
> +
> +	val = readl(priv->base + HSPHY_CFG1);
> +	val &= ~HSPHY_CFG1_ADR_EN;
> +	writel(val, priv->base + HSPHY_CFG1);
> +
> +	val = readl(priv->base + HSPHY_CFG1);
> +	val &= ~FIELD_PREP(HSPHY_CFG1_DAT_MASK, p->mask);
> +	val |=  FIELD_PREP(HSPHY_CFG1_DAT_MASK, p->val) | HSPHY_CFG1_DAT_EN;
> +	writel(val, priv->base + HSPHY_CFG1);
> +
> +	val = readl(priv->base + HSPHY_CFG1);
> +	val &= ~HSPHY_CFG1_DAT_EN;
> +	writel(val, priv->base + HSPHY_CFG1);
> +}
> +
> +static int uniphier_u3hsphy_init(struct phy *phy)
> +{
> +	struct uniphier_u3hsphy_priv *priv = phy_get_drvdata(phy);
> +	u32 config0, config1;
> +	int i, ret;
> +
> +	ret = clk_prepare_enable(priv->clk_phy_ext);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(priv->clk_phy);
> +	if (ret)
> +		goto out_clk_ext_disable;
> +
> +	ret = reset_control_deassert(priv->rst_phy);
> +	if (ret)
> +		goto out_clk_disable;
> +
> +	if (!priv->data->config0 && !priv->data->config1)
> +		return 0;
> +
> +	config0 = priv->data->config0;
> +	config1 = priv->data->config1;
> +
> +	ret = uniphier_u3hsphy_update_config(priv, &config0);
> +	if (ret)
> +		goto out_rst_assert;
> +
> +	writel(config0, priv->base + HSPHY_CFG0);
> +	writel(config1, priv->base + HSPHY_CFG1);
> +
> +	for (i = 0; i < priv->data->nparams; i++)
> +		uniphier_u3hsphy_set_param(priv, &priv->data->param[i]);
> +
> +	return 0;
> +
> +out_rst_assert:
> +	reset_control_assert(priv->rst_phy);
> +out_clk_disable:
> +	clk_disable_unprepare(priv->clk_phy);
> +out_clk_ext_disable:
> +	clk_disable_unprepare(priv->clk_phy_ext);
> +
> +	return ret;
> +}
> +
> +static int uniphier_u3hsphy_exit(struct phy *phy)
> +{
> +	struct uniphier_u3hsphy_priv *priv = phy_get_drvdata(phy);
> +
> +	reset_control_assert(priv->rst_phy);
> +	clk_disable_unprepare(priv->clk_phy);
> +	clk_disable_unprepare(priv->clk_phy_ext);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops uniphier_u3hsphy_ops = {
> +	.init           = uniphier_u3hsphy_init,
> +	.exit           = uniphier_u3hsphy_exit,
> +	.owner          = THIS_MODULE,
> +};
> +
> +static int uniphier_u3hsphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct uniphier_u3hsphy_priv *priv;
> +	struct phy_provider *phy_provider;
> +	struct resource *res;
> +	struct phy *phy;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	const char *name;
> +	int i, ret, nc, nr;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->data = of_device_get_match_data(dev);
> +	if (WARN_ON(!priv->data ||
> +		    priv->data->nparams > MAX_PHY_PARAMS))
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	for (i = 0; i < MAX_CLKS; i++) {
> +		name = priv->data->clock_names[i];
> +		if (!name)
> +			break;
> +		clk = devm_clk_get(dev, name);
> +		/* "phy-ext" is optional */
> +		if (!strcmp(name, "phy-ext")) {
> +			if (PTR_ERR(clk) == -ENOENT)
> +				clk = NULL;
> +			priv->clk_phy_ext = clk;
> +		} else if (!strcmp(name, "phy")) {
> +			priv->clk_phy = clk;
> +		} else {
> +			priv->clk[priv->nclks++] = clk;

Only "link" clock will be here? Do we need an array for this?

I think the above can be replaced with 3 separate clk_get calls instead of
storing clock names in uniphier_u3hsphy_soc_data.

Thanks
Kishon

Powered by blists - more mailing lists