Message-id: <52F39710.80101@samsung.com>
Date: Thu, 06 Feb 2014 15:07:12 +0100
From: Tomasz Figa <t.figa@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101
 Thunderbird/24.1.1
MIME-version: 1.0
To: Vivek Gautam <gautam.vivek@samsung.com>, linux-usb@vger.kernel.org,
        linux-samsung-soc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-doc@vger.kernel.org, gregkh@linuxfoundation.org,
        kgene.kim@samsung.com, balbi@ti.com, kishon@ti.com,
        k.debski@samsung.com, s.nawrocki@samsung.com, jwerner@chromium.org,
        jg1.han@samsung.com
Subject: Re: [PATCH v3] phy: Add new Exynos5 USB 3.0 PHY driver
References: <1386151747-3209-2-git-send-email-gautam.vivek@samsung.com>
 <1390225363-24210-1-git-send-email-gautam.vivek@samsung.com>
In-reply-to: <1390225363-24210-1-git-send-email-gautam.vivek@samsung.com>
Content-type: text/plain; charset=ISO-8859-1; format=flowed
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

Hi Vivek,

This patch is just adding the PHY driver. I would also like to look at 
some users of it, to see how this works when put together.

For now, please see my comments inline.

On 20.01.2014 14:42, Vivek Gautam wrote:
> Add a new driver for the USB 3.0 PHY on Exynos5 series of SoCs.
> The new driver uses the generic PHY framework and will interact
> with DWC3 controller present on Exynos5 series of SoCs.
> Thereby, removing old phy-samsung-usb3 driver and related code
> used untill now which was based on usb/phy framework.
>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>
> Changes from v2:
> 1) Added support for multiple PHYs (UTMI+ and PIPE3) and
>     related changes in the driver structuring.

I'm a bit skeptical about this separation. Can the PHY operate with just 
the UTMI+ or PIPE3 part enabled alone without the other? Can any PHY 
consumer operate this way?

Introducing separation of something that can't exist alone doesn't add 
any value, but instead makes things more difficult to work with. Of 
course, it's fine if the answer to my questions above if yes, but better 
safe than sorry.

> 2) Added a xlate function to get the required phy out of
>     number of PHYs in mutiple PHY scenerio.
> 3) Changed the names of few structures and variables to
>     have a clearer meaning.
> 4) Added 'usb3phy_config' structure to take care of mutiple
>     phys for a SoC having 'exynos5_usb3phy_drv_data' driver data.
> 5) Not deleting support for old driver 'phy-samsung-usb3' until
>     required support for generic phy is added to DWC3.

[snip]

> +
> +- aliases: For SoCs like Exynos5420 having multiple USB PHY controllers,
> +	   'usb3_phy' nodes should have numbered alias in the aliases node,
> +	   in the form of usb3phyN, N = 0, 1... (depending on number of
> +	   controllers).
> +Example:
> +	aliases {
> +		usb3phy0 = &usb3_phy0;
> +		usb3phy1 = &usb3_phy1;
> +	};

What is the reason to have these aliases?

> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 330ef2d..32f9f38 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -51,4 +51,12 @@ config PHY_EXYNOS_DP_VIDEO
>   	help
>   	  Support for Display Port PHY found on Samsung EXYNOS SoCs.
>
> +config PHY_EXYNOS5_USB3
> +	tristate "Exynos5 SoC series USB 3.0 PHY driver"
> +	depends on ARCH_EXYNOS5
> +	select GENERIC_PHY
> +	select MFD_SYSCON
> +	help
> +	  Enable USB 3.0 PHY support for Exynos 5 SoC series
> +
>   endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index d0caae9..9c06a61 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>   obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
>   obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>   obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
> +obj-$(CONFIG_PHY_EXYNOS5_USB3)		+= phy-exynos5-usb3.o
> diff --git a/drivers/phy/phy-exynos5-usb3.c b/drivers/phy/phy-exynos5-usb3.c
> new file mode 100644
> index 0000000..24efed0
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5-usb3.c
> @@ -0,0 +1,621 @@
> +/*
> + * Samsung EXYNOS5 SoC series USB 3.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Vivek Gautam <gautam.vivek@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +/* Exynos USB PHY registers */
> +#define EXYNOS5_FSEL_9MHZ6		0x0
> +#define EXYNOS5_FSEL_10MHZ		0x1
> +#define EXYNOS5_FSEL_12MHZ		0x2
> +#define EXYNOS5_FSEL_19MHZ2		0x3
> +#define EXYNOS5_FSEL_20MHZ		0x4
> +#define EXYNOS5_FSEL_24MHZ		0x5
> +#define EXYNOS5_FSEL_50MHZ		0x7
> +
> +/* EXYNOS5: USB 3.0 DRD PHY registers */
> +#define EXYNOS5_DRD_LINKSYSTEM			(0x04)

nit: No need for parentheses around simple literal. (+ more occurrences 
below)

> +
> +#define LINKSYSTEM_FLADJ_MASK			(0x3f << 1)
> +#define LINKSYSTEM_FLADJ(_x)			((_x) << 1)
> +#define LINKSYSTEM_XHCI_VERSION_CONTROL		(0x1 << 27)

nit: BIT() macro could be used for single bits. (+ more occurrences below)

> +
> +#define EXYNOS5_DRD_PHYUTMI			(0x08)
> +
> +#define PHYUTMI_OTGDISABLE			(0x1 << 6)
> +#define PHYUTMI_FORCESUSPEND			(0x1 << 1)
> +#define PHYUTMI_FORCESLEEP			(0x1 << 0)
> +
> +#define EXYNOS5_DRD_PHYPIPE			(0x0c)
> +
> +#define EXYNOS5_DRD_PHYCLKRST			(0x10)
> +
> +#define PHYCLKRST_EN_UTMISUSPEND		(0x1 << 31)
> +
> +#define PHYCLKRST_SSC_REFCLKSEL_MASK		(0xff << 23)
> +#define PHYCLKRST_SSC_REFCLKSEL(_x)		((_x) << 23)
> +
> +#define PHYCLKRST_SSC_RANGE_MASK		(0x03 << 21)
> +#define PHYCLKRST_SSC_RANGE(_x)			((_x) << 21)
> +
> +#define PHYCLKRST_SSC_EN			(0x1 << 20)
> +#define PHYCLKRST_REF_SSP_EN			(0x1 << 19)
> +#define PHYCLKRST_REF_CLKDIV2			(0x1 << 18)
> +
> +#define PHYCLKRST_MPLL_MULTIPLIER_MASK		(0x7f << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF	(0x19 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF	(0x32 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF	(0x68 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF	(0x7d << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF	(0x02 << 11)
> +
> +#define PHYCLKRST_FSEL_MASK			(0x3f << 5)
> +#define PHYCLKRST_FSEL(_x)			((_x) << 5)
> +#define PHYCLKRST_FSEL_PAD_100MHZ		(0x27 << 5)
> +#define PHYCLKRST_FSEL_PAD_24MHZ		(0x2a << 5)
> +#define PHYCLKRST_FSEL_PAD_20MHZ		(0x31 << 5)
> +#define PHYCLKRST_FSEL_PAD_19_2MHZ		(0x38 << 5)
> +
> +#define PHYCLKRST_RETENABLEN			(0x1 << 4)
> +
> +#define PHYCLKRST_REFCLKSEL_MASK		(0x03 << 2)
> +#define PHYCLKRST_REFCLKSEL_PAD_REFCLK		(0x2 << 2)
> +#define PHYCLKRST_REFCLKSEL_EXT_REFCLK		(0x3 << 2)
> +
> +#define PHYCLKRST_PORTRESET			(0x1 << 1)
> +#define PHYCLKRST_COMMONONN			(0x1 << 0)
> +
> +#define EXYNOS5_DRD_PHYREG0			(0x14)
> +#define EXYNOS5_DRD_PHYREG1			(0x18)
> +
> +#define EXYNOS5_DRD_PHYPARAM0			(0x1c)
> +
> +#define PHYPARAM0_REF_USE_PAD			(0x1 << 31)
> +#define PHYPARAM0_REF_LOSLEVEL_MASK		(0x1f << 26)
> +#define PHYPARAM0_REF_LOSLEVEL			(0x9 << 26)
> +
> +#define EXYNOS5_DRD_PHYPARAM1			(0x20)
> +
> +#define PHYPARAM1_PCS_TXDEEMPH_MASK		(0x1f << 0)
> +#define PHYPARAM1_PCS_TXDEEMPH			(0x1c)
> +
> +#define EXYNOS5_DRD_PHYTERM			(0x24)
> +
> +#define EXYNOS5_DRD_PHYTEST			(0x28)
> +
> +#define PHYTEST_POWERDOWN_SSP			(0x1 << 3)
> +#define PHYTEST_POWERDOWN_HSP			(0x1 << 2)
> +
> +#define EXYNOS5_DRD_PHYADP			(0x2c)
> +
> +#define EXYNOS5_DRD_PHYBATCHG			(0x30)
> +
> +#define PHYBATCHG_UTMI_CLKSEL			(0x1 << 2)
> +
> +#define EXYNOS5_DRD_PHYRESUME			(0x34)
> +#define EXYNOS5_DRD_LINKPORT			(0x44)
> +
> +/* Power isolation defined in power management unit */
> +#define EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET	(0x704)
> +#define EXYNOS5_USB3DRD_PMU_ISOL		(1 << 0)
> +
> +#define KHZ	1000
> +#define MHZ	(KHZ * KHZ)
> +
> +enum exynos5_usb3phy_id {
> +	EXYNOS5_USB3PHY_UTMI,
> +	EXYNOS5_USB3PHY_PIPE3,
> +	EXYNOS5_USB3PHYS_NUM,
> +};
> +
> +struct usb3phy_config {
> +	u32 id;
> +	u32 reg_pmu_offset;
> +	void (*phy_isol)(struct phy *phy, u32 on);
> +};
> +
> +struct exynos5_usb3phy_drv_data {
> +	bool has_usb30_sclk;
> +	bool has_multi_controller;
> +	const struct usb3phy_config *phy_cfg;
> +};
> +
> +/**
> + * struct exynos5_usb3phy_driver - driver data for USB 3.0 PHY
> + * @dev: pointer to device instance of this platform device
> + * @reg_phy: usb phy controller register memory base
> + * @clk: phy clock for register access
> + * @usb30_sclk: additional special clock for phy operations
> + * @drv_data: pointer to SoC level driver data structure
> + * @phys[]: array for 'EXYNOS5_USB3PHYS_NUM' number of PHY
> + *	    instances each with its 'phy' and 'phy_cfg'.
> + * @extrefclk: frequency select settings when using 'separate
> + *	       reference clocks' for SS and HS operations
> + * @rate: rate of reference clock to PHY block
> + * @channel: number of PHY channels present in SoC
> + */
> +struct exynos5_usb3phy_driver {
> +	struct device *dev;
> +	void __iomem *reg_phy;
> +	struct clk *clk;
> +	struct clk *usb30_sclk;
> +	const struct exynos5_usb3phy_drv_data *drv_data;
> +	struct phy_usb_instance {
> +		struct phy *phy;
> +		u32 index;
> +		struct regmap *reg_isol;
> +		const struct usb3phy_config *phy_cfg;
> +	} phys[EXYNOS5_USB3PHYS_NUM];
> +	u32 extrefclk;
> +	unsigned long rate;
> +	u32 channel;
> +};
> +
> +#define to_usb3phy_driver(inst) \
> +	container_of((inst), struct exynos5_usb3phy_driver, \
> +		     phys[(inst)->index]);
> +
> +/*
> + * exynos5_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register.
> + */
> +static u32 exynos5_rate_to_clk(unsigned long rate)
> +{
> +	unsigned int clksel;
> +
> +	/* EXYNOS5_FSEL_MASK */
> +
> +	switch (rate) {
> +	case 9600 * KHZ:
> +		clksel = EXYNOS5_FSEL_9MHZ6;
> +		break;
> +	case 10 * MHZ:
> +		clksel = EXYNOS5_FSEL_10MHZ;
> +		break;
> +	case 12 * MHZ:
> +		clksel = EXYNOS5_FSEL_12MHZ;
> +		break;
> +	case 19200 * KHZ:
> +		clksel = EXYNOS5_FSEL_19MHZ2;
> +		break;
> +	case 20 * MHZ:
> +		clksel = EXYNOS5_FSEL_20MHZ;
> +		break;
> +	case 24 * MHZ:
> +		clksel = EXYNOS5_FSEL_24MHZ;
> +		break;
> +	case 50 * MHZ:
> +		clksel = EXYNOS5_FSEL_50MHZ;
> +		break;
> +	default:
> +		clksel = -EINVAL;
> +	}
> +
> +	return clksel;
> +}
> +
> +static void exynos5_usb3phy_isol(struct phy *phy, unsigned int on)
> +{
> +	u32 pmu_offset;
> +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
> +	struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
> +
> +	pmu_offset = inst->phy_cfg->reg_pmu_offset;
> +	if (!inst->reg_isol)
> +		return;
> +
> +	switch (drv->channel) {
> +	case 1:
> +		/* Channel 1 is at 0x708 offset */
> +		pmu_offset += sizeof(&pmu_offset);
> +		break;
> +	case 0:
> +	default:
> +		/* Channel 0 is at 0x704 offset */
> +		break;
> +	}
> +
> +	regmap_update_bits(inst->reg_isol, pmu_offset,
> +			   EXYNOS5_USB3DRD_PMU_ISOL, ~on);
> +}
> +
> +/*
> + * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
> + */
> +static u32 exynos5_usb3phy_set_refclk(struct exynos5_usb3phy_driver *drv)
> +{
> +	u32 reg;
> +
> +	reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
> +	      PHYCLKRST_FSEL(drv->extrefclk);
> +
> +	switch (drv->extrefclk) {
> +	case EXYNOS5_FSEL_50MHZ:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x00));
> +		break;
> +	case EXYNOS5_FSEL_24MHZ:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x88));
> +		break;
> +	case EXYNOS5_FSEL_20MHZ:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x00));
> +		break;
> +	case EXYNOS5_FSEL_19MHZ2:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x88));
> +		break;
> +	default:
> +		dev_dbg(drv->dev, "unsupported ref clk\n");
> +		break;
> +	}
> +
> +	return reg;
> +}
> +
> +static int exynos5_usb3phy_init(struct phy *phy)
> +{
> +	int ret;
> +	u32 phyparam0;
> +	u32 phyparam1;
> +	u32 linksystem;
> +	u32 phybatchg;
> +	u32 phytest;
> +	u32 phyclkrst;
> +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
> +	struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
> +
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		return ret;
> +
> +	drv->extrefclk = exynos5_rate_to_clk(drv->rate);
> +	if (drv->extrefclk == -EINVAL) {
> +		dev_err(drv->dev, "Clock rate (%ld) not supported\n",
> +						drv->rate);
> +		return -EINVAL;
> +	}
> +
> +	/* Reset USB 3.0 PHY */
> +	writel(0x0, drv->reg_phy + EXYNOS5_DRD_PHYREG0);
> +
> +	phyparam0 = readl(drv->reg_phy + EXYNOS5_DRD_PHYPARAM0);
> +	/* Select PHY CLK source */
> +	phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
> +	/* Set Loss-of-Signal Detector sensitivity */
> +	phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
> +	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
> +	writel(phyparam0, drv->reg_phy + EXYNOS5_DRD_PHYPARAM0);
> +
> +	writel(0x0, drv->reg_phy + EXYNOS5_DRD_PHYRESUME);
> +
> +	/*
> +	 * Setting the Frame length Adj value[6:1] to default 0x20
> +	 * See xHCI 1.0 spec, 5.2.4
> +	 */
> +	linksystem = LINKSYSTEM_XHCI_VERSION_CONTROL |
> +		     LINKSYSTEM_FLADJ(0x20);
> +	writel(linksystem, drv->reg_phy + EXYNOS5_DRD_LINKSYSTEM);
> +
> +	phyparam1 = readl(drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
> +	/* Set Tx De-Emphasis level */
> +	phyparam1 &= ~PHYPARAM1_PCS_TXDEEMPH_MASK;
> +	phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
> +	writel(phyparam1, drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
> +
> +	phybatchg = readl(drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
> +	phybatchg |= PHYBATCHG_UTMI_CLKSEL;
> +	writel(phybatchg, drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
> +
> +	/* PHYTEST POWERDOWN Control */
> +	phytest = readl(drv->reg_phy + EXYNOS5_DRD_PHYTEST);
> +	phytest &= ~(PHYTEST_POWERDOWN_SSP |
> +		     PHYTEST_POWERDOWN_HSP);
> +	writel(phytest, drv->reg_phy + EXYNOS5_DRD_PHYTEST);
> +
> +	/* UTMI Power Control */
> +	writel(PHYUTMI_OTGDISABLE, drv->reg_phy + EXYNOS5_DRD_PHYUTMI);
> +
> +	phyclkrst = exynos5_usb3phy_set_refclk(drv);
> +
> +	phyclkrst |= PHYCLKRST_PORTRESET |
> +		     /* Digital power supply in normal operating mode */
> +		     PHYCLKRST_RETENABLEN |
> +		     /* Enable ref clock for SS function */
> +		     PHYCLKRST_REF_SSP_EN |
> +		     /* Enable spread spectrum */
> +		     PHYCLKRST_SSC_EN |
> +		     /* Power down HS Bias and PLL blocks in suspend mode */
> +		     PHYCLKRST_COMMONONN;
> +
> +	writel(phyclkrst, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
> +
> +	udelay(10);
> +
> +	phyclkrst &= ~PHYCLKRST_PORTRESET;
> +	writel(phyclkrst, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
> +
> +	clk_disable_unprepare(drv->clk);

I'm still not convinced that this is the right place for this setup. 
This way you force consumer driver to always call phy_power_on() and 
phy_init() together, otherwise the PHY won't work.

I believe the right thing to do here is to do all the initialization in 
.power_on() and let the driver simply call phy_power_on() when it needs 
the PHY and phy_power_off() otherwise.

Analogically, .exit() should be merged into .power_off().

> +
> +	return 0;
> +}

[snip]

> +	/*
> +	 * Exynos5420 SoC has multiple channels for USB 3.0 PHY, with
> +	 * each having separate power control registers.
> +	 * 'drv->channel' facilitates to set such registers.
> +	 */
> +	if (drv->drv_data->has_multi_controller) {
> +		drv->channel = of_alias_get_id(node, "usb3phy");
> +		if (drv->channel < 0) {
> +			dev_err(dev, "Invalid usb3drd phy node\n");
> +			return -EINVAL;
> +		}
> +	}

Aha, so this is why you need aliases. Maybe a "samsung,pmu-offset" 
property, simply specifying the offset to PMU regs would be better here?

> +
> +	drv->clk = devm_clk_get(dev, "phy");
> +	if (IS_ERR(drv->clk)) {
> +		dev_err(dev, "Failed to get clock of phy controller\n");
> +		return PTR_ERR(drv->clk);
> +	}
> +
> +	/*
> +	 * Exysno5420 SoC has an additional special clock, used for
> +	 * for USB 3.0 PHY operation, this clock goes to the PHY block
> +	 * as a reference clock to clock generation block of the controller,
> +	 * named as 'USB30_SCLK_100M'.
> +	 */
> +	if (drv_data->has_usb30_sclk) {
> +		drv->usb30_sclk = devm_clk_get(dev, "usb30_sclk_100m");
> +		if (IS_ERR(drv->usb30_sclk)) {
> +			dev_err(dev, "Failed to get phy reference clock\n");
> +			return PTR_ERR(drv->usb30_sclk);
> +		}
> +	}
> +
> +	clk = clk_get(dev, "usb3phy_refclk");
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "Failed to get reference clock of usb3drd phy\n");
> +		return PTR_ERR(clk);
> +	}
> +	drv->rate = clk_get_rate(clk);
> +	clk_put(clk);

To comply with clock API semantics, you should keep the reference on 
this clock until the driver is removed. Moreover, I believe you should 
call clk_prepare_enable() on it as well, to make sure that the clock is 
enabled, even if current implementation of clock drivers for Exynos 5 
can't disable this clock.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/