[v4,3/9] phy: Add new Exynos USB PHY driver

Add a new driver for the Exynos USB PHY. The new driver uses the generic
PHY framework. The driver includes support for the Exynos 4x10 and 4x12
SoC families.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/phy/samsung-usbphy.txt     |   54 ++++
 drivers/phy/Kconfig                                |   20 ++
 drivers/phy/Makefile                               |    3 +
 drivers/phy/phy-exynos4210-usb2.c                  |  264 +++++++++++++++++
 drivers/phy/phy-exynos4212-usb2.c                  |  312 ++++++++++++++++++++
 drivers/phy/phy-samsung-usb2.c                     |  228 ++++++++++++++
 drivers/phy/phy-samsung-usb2.h                     |   72 +++++
 7 files changed, 953 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/samsung-usbphy.txt
 create mode 100644 drivers/phy/phy-exynos4210-usb2.c
 create mode 100644 drivers/phy/phy-exynos4212-usb2.c
 create mode 100644 drivers/phy/phy-samsung-usb2.c
 create mode 100644 drivers/phy/phy-samsung-usb2.h

Hi,

On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
> Add a new driver for the Exynos USB PHY. The new driver uses the generic
> PHY framework. The driver includes support for the Exynos 4x10 and 4x12
> SoC families.
> 
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/phy/samsung-usbphy.txt     |   54 ++++
>  drivers/phy/Kconfig                                |   20 ++
>  drivers/phy/Makefile                               |    3 +
>  drivers/phy/phy-exynos4210-usb2.c                  |  264 +++++++++++++++++
>  drivers/phy/phy-exynos4212-usb2.c                  |  312 ++++++++++++++++++++
>  drivers/phy/phy-samsung-usb2.c                     |  228 ++++++++++++++
>  drivers/phy/phy-samsung-usb2.h                     |   72 +++++
>  7 files changed, 953 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/samsung-usbphy.txt
>  create mode 100644 drivers/phy/phy-exynos4210-usb2.c
>  create mode 100644 drivers/phy/phy-exynos4212-usb2.c
>  create mode 100644 drivers/phy/phy-samsung-usb2.c
>  create mode 100644 drivers/phy/phy-samsung-usb2.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> new file mode 100644
> index 0000000..cadbf70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt

use the existing samsung-phy.txt.
> @@ -0,0 +1,54 @@
> +Samsung S5P/EXYNOS SoC series USB PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-usb2-phy"
> +	- "samsung,exynos4212-usb2-phy"
> +- reg : a list of registers used by phy driver
> +	- first and obligatory is the location of phy modules registers
> +- samsung,sysreg-phandle - handle to syscon used to control the system registers
> +- samsung,pmureg-phandle - handle to syscon used to control PMU registers
> +- #phy-cells : from the generic phy bindings, must be 1;
> +- clocks and clock-names:
> +	- the "phy" clocks is required by the phy module
> +	- next for each of the phys a clock has to be assidned, this clock

%s/assidned/assigned/
> +	  will be used to determine clocking frequency for the phys
> +	  (the labels are specified in the paragraph below)
> +
> +The first phandle argument in the PHY specifier identifies the PHY, its
> +meaning is compatible dependent. For the currently supported SoCs (Exynos 4210
> +and Exynos 4212) it is as follows:
> +  0 - USB device ("device"),
> +  1 - USB host ("host"),
> +  2 - HSIC0 ("hsic0"),
> +  3 - HSIC1 ("hsic1"),
> +
> +Exynos 4210 and Exynos 4212 use mode switching and require that mode switch
> +register is supplied.
> +
> +Example:
> +
> +For Exynos 4412 (compatible with Exynos 4212):
> +
> +usbphy: phy@125B0000 {

use lower case for address here...
> +	compatible = "samsung,exynos4212-usb2-phy";
> +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
and here..
> +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> +							<&clock 2>;
> +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> +	status = "okay";
> +	#phy-cells = <1>;
> +	samsung,sysreg-phandle = <&sys_reg>;
> +	samsung,pmureg-phandle = <&pmu_reg>;
> +};
> +
> +Then the PHY can be used in other nodes such as:
> +
> +phy-consumer@12340000 {
> +	phys = <&usbphy 2>;
> +	phy-names = "phy";
> +};
> +
> +Refer to DT bindings documentation of particular PHY consumer devices for more
> +information about required PHYs and the way of specification.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index a344f3d..b29018f 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
>  	help
>  	  Support for Display Port PHY found on Samsung EXYNOS SoCs.
>  
> +config PHY_SAMSUNG_USB2
> +	tristate "Samsung USB 2.0 PHY driver"
> +	help
> +	  Enable this to support Samsung USB phy helper driver for Samsung SoCs.
> +	  This driver provides common interface to interact, for Samsung
> +	  USB 2.0 PHY driver.
> +
> +config PHY_EXYNOS4210_USB2
> +	bool "Support for Exynos 4210"
> +	depends on PHY_SAMSUNG_USB2
> +	depends on CPU_EXYNOS4210

select GENERIC_PHY here?
> +	help
> +	  Enable USB PHY support for Exynos 4210

Add more explanation here and make checkpatch happy.
> +
> +config PHY_EXYNOS4212_USB2
> +	bool "Support for Exynos 4212"
> +	depends on PHY_SAMSUNG_USB2
> +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)

select GENERIC_PHY.
> +	help
> +	  Enable USB PHY support for Exynos 4212

more explanation here too..
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index d0caae9..9f4befd 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
> +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
> +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
> diff --git a/drivers/phy/phy-exynos4210-usb2.c b/drivers/phy/phy-exynos4210-usb2.c
> new file mode 100644
> index 0000000..a02e5c2
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4210-usb2.c
> @@ -0,0 +1,264 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/regmap.h>
> +#include <linux/spinlock.h>

You've included most of the above header files in phy-samsung-usb2.h which you
are including below.
> +#include "phy-samsung-usb2.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4210_UPHYPWR			0x0
> +
> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)

use BIT() here and everywhere below.
> +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)

replace PHY0 here with DEV so it looks similar to EXYNOS_4212.
> +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)

replace PHY0 here with HOST so it looks similar to EXYNOS_4212.
> +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4210_UPHYCLK			0x4
> +
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +
> +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +/* PHY reset control */
> +#define EXYNOS_4210_UPHYRST			0x8
> +
> +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x704
> +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x708
> +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> +
> +/* USBYPHY1 Floating prevention */
> +#define EXYNOS_4210_UPHY1CON			0x34
> +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> +
> +/* Mode switching SUB Device <-> Host */
> +#define EXYNOS_4210_MODE_SWITCH_OFFSET		0x21c
> +#define EXYNOS_4210_MODE_SWITCH_MASK		1
> +#define EXYNOS_4210_MODE_SWITCH_DEVICE		0
> +#define EXYNOS_4210_MODE_SWITCH_HOST		1
> +
> +enum exynos4210_phy_id {
> +	EXYNOS4210_DEVICE,
> +	EXYNOS4210_HOST,
> +	EXYNOS4210_HSIC0,
> +	EXYNOS4210_HSIC1,
> +	EXYNOS4210_NUM_PHYS,
> +};
> +
> +/*
> + * exynos4210_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register.
> + */
> +static int exynos4210_rate_to_clk(unsigned long rate, u32 *reg)
> +{
> +	switch (rate) {
> +	case 12 * MHZ:
> +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 24 * MHZ:
> +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 48 * MHZ:
> +		reg = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;

%s/reg/*reg
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void exynos4210_isol(struct samsung_usb2_phy_instance *inst, bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> +		break;
> +	case EXYNOS4210_HOST:
> +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_HOST;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask);
> +}
> +
> +static void exynos4210_phy_pwr(struct samsung_usb2_phy_instance *inst, bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> +		break;
> +	case EXYNOS4210_HOST:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> +				EXYNOS_4210_URSTCON_PHY1_P0 |
> +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> +		break;
> +	case EXYNOS4210_HSIC0:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> +		break;
> +	case EXYNOS4210_HSIC1:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk_reg_val, drv->reg_phy + EXYNOS_4210_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);

Do you have to reset during every power on? IMO this reset should be done once
in phy_init.
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4210_power_on(struct samsung_usb2_phy_instance *inst)
> +{
> +	/* Order of initialisation is important - first power then isolation */
> +	exynos4210_phy_pwr(inst, 1);
> +	exynos4210_isol(inst, 0);
> +
> +	return 0;
> +}
> +
> +static int exynos4210_power_off(struct samsung_usb2_phy_instance *inst)
> +{
> +	exynos4210_isol(inst, 1);
> +	exynos4210_phy_pwr(inst, 0);
> +
> +	return 0;
> +}
> +
> +
> +static const struct samsung_usb2_common_phy exynos4210_phys[] = {
> +	{
> +		.label		= "device",
> +		.id		= EXYNOS4210_DEVICE,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.id		= EXYNOS4210_HOST,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.id		= EXYNOS4210_HSIC0,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.id		= EXYNOS4210_HSIC1,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{},
> +};
> +
> +const struct samsung_usb2_phy_config exynos4210_usb2_phy_config = {
> +	.num_phys		= EXYNOS4210_NUM_PHYS,
> +	.phys			= exynos4210_phys,
> +	.has_mode_switch	= 1,
> +};
> +
> diff --git a/drivers/phy/phy-exynos4212-usb2.c b/drivers/phy/phy-exynos4212-usb2.c
> new file mode 100644
> index 0000000..375ece0
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4212-usb2.c
> @@ -0,0 +1,312 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver - Exynos 4212 support
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/regmap.h>
> +#include <linux/spinlock.h>

You've included most of the above header files in phy-samsung-usb2.h which you
are including below.
> +#include "phy-samsung-usb2.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4212_UPHYPWR			0x0
> +
> +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)

use BIT() here and below..
> +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> +#define EXYNOS_4212_UPHYPWR_DEV	( \
> +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> +#define EXYNOS_4212_UPHYPWR_HOST ( \
> +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4212_UPHYCLK			0x4
> +
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> +
> +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> +
> +/* PHY reset control */
> +#define EXYNOS_4212_UPHYRST			0x8
> +
> +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4212_USB_ISOL_OFFSET		0x704
> +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x708
> +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x70c
> +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> +
> +/* Mode switching SUB Device <-> Host */
> +#define EXYNOS_4212_MODE_SWITCH_OFFSET		0x21c
> +#define EXYNOS_4212_MODE_SWITCH_MASK		1
> +#define EXYNOS_4212_MODE_SWITCH_DEVICE		0
> +#define EXYNOS_4212_MODE_SWITCH_HOST		1
> +
> +enum exynos4x12_phy_id {
> +	EXYNOS4212_DEVICE,
> +	EXYNOS4212_HOST,
> +	EXYNOS4212_HSIC0,
> +	EXYNOS4212_HSIC1,
> +	EXYNOS4212_NUM_PHYS,
> +};
> +
> +/*
> + * exynos4212_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register.
> + */
> +static int exynos4212_rate_to_clk(unsigned long rate, u32 *reg)
> +{
> +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> +
> +	switch (rate) {
> +	case 9600 * KHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> +		break;
> +	case 10 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> +		break;
> +	case 12 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 19200 * KHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> +		break;
> +	case 20 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> +		break;
> +	case 24 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 50 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void exynos4212_isol(struct samsung_usb2_phy_instance *inst, bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +	case EXYNOS4212_HOST:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask);
> +}
> +
> +static void exynos4212_phy_pwr(struct samsung_usb2_phy_instance *inst, bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> +		break;
> +	case EXYNOS4212_HOST:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> +				EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk_reg_val, drv->reg_phy + EXYNOS_4212_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);

reset should be done once in init?
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4212_power_on(struct samsung_usb2_phy_instance *inst)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +
> +	inst->enabled = 1;
> +	exynos4212_phy_pwr(inst, 1);
> +	exynos4212_isol(inst, 0);
> +
> +	/* Power on the device, as it is necessary for HSIC to work */

This looks weird. How powering on the 'device PHY' makes 'HSIC PHY' to work?
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct samsung_usb2_phy_instance *device =
> +					&drv->instances[EXYNOS4212_DEVICE];
> +		exynos4212_phy_pwr(device, 1);
> +		exynos4212_isol(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int exynos4212_power_off(struct samsung_usb2_phy_instance *inst)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	struct samsung_usb2_phy_instance *device = &drv->instances[EXYNOS4212_DEVICE];
> +
> +	inst->enabled = 0;
> +	exynos4212_isol(inst, 1);
> +	exynos4212_phy_pwr(inst, 0);
> +
> +	if (inst->cfg->id == EXYNOS4212_HSIC0 && !device->enabled) {
> +		exynos4212_isol(device, 1);
> +		exynos4212_phy_pwr(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static const struct samsung_usb2_common_phy exynos4212_phys[] = {
> +	{
> +		.label		= "device",
> +		.id		= EXYNOS4212_DEVICE,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.id		= EXYNOS4212_HOST,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.id		= EXYNOS4212_HSIC0,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.id		= EXYNOS4212_HSIC1,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{},
> +};
> +
> +const struct samsung_usb2_phy_config exynos4212_usb2_phy_config = {
> +	.num_phys		= EXYNOS4212_NUM_PHYS,
> +	.phys			= exynos4212_phys,
> +	.has_mode_switch	= 1,
> +};
> +
> diff --git a/drivers/phy/phy-samsung-usb2.c b/drivers/phy/phy-samsung-usb2.c
> new file mode 100644
> index 0000000..804ec77
> --- /dev/null
> +++ b/drivers/phy/phy-samsung-usb2.c
> @@ -0,0 +1,228 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/mfd/syscon.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/spinlock.h>

You've included most of the above header files in phy-samsung-usb2.h which you
are including below.
> +#include "phy-samsung-usb2.h"
> +
> +static int samsung_usb2_phy_power_on(struct phy *phy)
> +{
> +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_dbg(drv->dev, "Request to power_on \"%s\" usb phy\n",
> +							inst->cfg->label);
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		goto err_main_clk;
> +	ret = clk_prepare_enable(inst->clk);
> +	if (ret)
> +		goto err_instance_clk;
> +	inst->rate = clk_get_rate(inst->clk);
> +	if (inst->cfg->rate_to_clk) {
> +		ret = inst->cfg->rate_to_clk(inst->rate, &inst->clk_reg_val);
> +		if (ret)
> +			goto err_get_rate;
> +	}
> +	if (inst->cfg->power_on) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_on(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +
> +	return 0;
> +
> +err_get_rate:
> +	clk_disable_unprepare(inst->clk);
> +err_instance_clk:
> +	clk_disable_unprepare(drv->clk);
> +err_main_clk:
> +	return ret;
> +}
> +
> +static int samsung_usb2_phy_power_off(struct phy *phy)
> +{
> +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	int ret = 0;
> +
> +	dev_dbg(drv->dev, "Request to power_off \"%s\" usb phy\n",
> +							inst->cfg->label);
> +	if (inst->cfg->power_off) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_off(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(inst->clk);
> +	clk_disable_unprepare(drv->clk);
> +	return ret;
> +}
> +
> +static struct phy_ops samsung_usb2_phy_ops = {
> +	.power_on	= samsung_usb2_phy_power_on,
> +	.power_off	= samsung_usb2_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *samsung_usb2_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct samsung_usb2_phy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->instances[args->args[0]].phy;
> +}
> +
> +static const struct of_device_id samsung_usb2_phy_of_match[] = {
> +#ifdef CONFIG_PHY_EXYNOS4210_USB2
> +	{
> +		.compatible = "samsung,exynos4210-usb2-phy",
> +		.data = &exynos4210_usb2_phy_config,
> +	},
> +#endif
> +#ifdef CONFIG_PHY_EXYNOS4212_USB2
> +	{
> +		.compatible = "samsung,exynos4212-usb2-phy",
> +		.data = &exynos4212_usb2_phy_config,
> +	},
> +#endif
> +	{ },
> +};

I think we've had enough discussion about this approach. Let's get the opinion
of others too. Felipe? Greg?

Summary:
We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with almost
similar register map [1] and a samsung helper driver for these two drivers.
These two PHY drivers populate the function pointers in the helper driver. So
any phy_ops will first invoke the helper driver which will then invoke the
corresponding phy driver.

[1] -> http://www.diffchecker.com/7yno1uvk

Advantages:
* (more) clean and readable
* helper driver can be used with other PHY drivers which will be added soon

Disadvantages:
* code duplication

Maybe having a helper driver makes sense when we have other samsung PHY drivers
added but not sure if it's needed here for EXYNOS4210_USB2 and EXYNOS4212_USB2

Need your inputs on what you think about this.
> +
> +static int samsung_usb2_phy_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct samsung_usb2_phy_config *cfg;
> +	struct clk *clk;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct resource *mem;
> +	struct samsung_usb2_phy_driver *drv;
> +	int i;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(dev, "This driver is required to be instantiated from device tree\n");
> +		return -EINVAL;
> +	}
> +
> +	match = of_match_node(samsung_usb2_phy_of_match, pdev->dev.of_node);
> +	if (!match) {
> +		dev_err(dev, "of_match_node() failed\n");
> +		return -EINVAL;
> +	}
> +	cfg = match->data;
> +
> +	drv = devm_kzalloc(dev, sizeof(struct samsung_usb2_phy_driver) +
> +		cfg->num_phys * sizeof(struct samsung_usb2_phy_instance), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, drv);
> +	spin_lock_init(&drv->lock);
> +
> +	drv->cfg = cfg;
> +	drv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_phy)) {
> +		dev_err(dev, "Failed to map register memory (phy)\n");
> +		return PTR_ERR(drv->reg_phy);
> +	}
> +
> +	drv->reg_pmu = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +		"samsung,pmureg-phandle");
> +	if (IS_ERR(drv->reg_pmu)) {
> +		dev_err(dev, "Failed to map PMU registers (via syscon)\n");
> +		return PTR_ERR(drv->reg_pmu);
> +	}
> +
> +	if (drv->cfg->has_mode_switch) {
> +		drv->reg_sys = syscon_regmap_lookup_by_phandle(
> +				pdev->dev.of_node, "samsung,sysreg-phandle");
> +		if (IS_ERR(drv->reg_sys)) {
> +			dev_err(dev, "Failed to map system registers (via syscon)\n");
> +			return PTR_ERR(drv->reg_sys);
> +		}
> +	}
> +
> +	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);
> +	}
> +
> +	for (i = 0; i < drv->cfg->num_phys; i++) {
> +		char *label = drv->cfg->phys[i].label;
> +		struct samsung_usb2_phy_instance *p = &drv->instances[i];
> +
> +		dev_dbg(dev, "Creating phy \"%s\"\n", label);
> +		p->phy = devm_phy_create(dev, &samsung_usb2_phy_ops, NULL);
> +		if (IS_ERR(p->phy)) {
> +			dev_err(drv->dev, "Failed to create usb2_phy \"%s\"\n",
> +						label);
> +			return PTR_ERR(p->phy);
> +		}
> +
> +		p->cfg = &drv->cfg->phys[i];
> +		p->drv = drv;
> +		phy_set_drvdata(p->phy, p);
> +
> +		clk = devm_clk_get(dev, p->cfg->label);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> +								p->cfg->label);
> +			return PTR_ERR(clk);
> +		}
> +		p->clk = clk;
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev,
> +							samsung_usb2_phy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(drv->dev, "Failed to register phy provider\n");
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver samsung_usb2_phy_driver = {
> +	.probe	= samsung_usb2_phy_probe,
> +	.driver = {
> +		.of_match_table	= samsung_usb2_phy_of_match,
> +		.name		= "samsung-usb2-phy",
> +		.owner		= THIS_MODULE,
> +	}
> +};
> +
> +module_platform_driver(samsung_usb2_phy_driver);
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> +MODULE_AUTHOR("Kamil Debski <k.debski@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:samsung-usb2-phy");
> +
> diff --git a/drivers/phy/phy-samsung-usb2.h b/drivers/phy/phy-samsung-usb2.h
> new file mode 100644
> index 0000000..cd12477
> --- /dev/null
> +++ b/drivers/phy/phy-samsung-usb2.h
> @@ -0,0 +1,72 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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.
> + */
> +
> +#ifndef _PHY_EXYNOS_USB2_H
> +#define _PHY_EXYNOS_USB2_H
> +
> +#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/spinlock.h>
> +
> +#define KHZ 1000
> +#define MHZ (KHZ * KHZ)
> +
> +struct samsung_usb2_phy_driver;
> +struct samsung_usb2_phy_instance;
> +struct samsung_usb2_phy_config;
> +
> +struct samsung_usb2_phy_instance {
> +	struct samsung_usb2_phy_driver *drv;
> +	struct phy *phy;
> +	const struct samsung_usb2_common_phy *cfg;
> +	char enabled;
> +	struct clk *clk;
> +	u32 clk_reg_val;
> +	unsigned long rate;
> +};
> +
> +struct samsung_usb2_phy_driver {
> +	struct device *dev;
> +	spinlock_t lock;
> +	void __iomem *reg_phy;
> +	struct regmap *reg_sys;
> +	struct regmap *reg_pmu;
> +	const struct samsung_usb2_phy_config *cfg;
> +	struct clk *clk;
> +	struct samsung_usb2_phy_instance instances[0];
> +};
> +
> +struct samsung_usb2_common_phy {
> +	char *label;
> +	unsigned int id;
> +	int (*rate_to_clk)(unsigned long, u32 *);
> +	int (*power_on)(struct samsung_usb2_phy_instance *);
> +	int (*power_off)(struct samsung_usb2_phy_instance *);
> +};
> +
> +
> +struct samsung_usb2_phy_config {
> +	int num_phys;
> +	const struct samsung_usb2_common_phy *phys;
> +	char has_mode_switch;

u8 instead?

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Kishon,

Thank you for the review.

> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> Sent: Friday, December 06, 2013 11:59 AM
> 
> Hi,
> 
> On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
> > Add a new driver for the Exynos USB PHY. The new driver uses the
> > generic PHY framework. The driver includes support for the Exynos
> 4x10
> > and 4x12 SoC families.
> >
> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  .../devicetree/bindings/phy/samsung-usbphy.txt     |   54 ++++
> >  drivers/phy/Kconfig                                |   20 ++
> >  drivers/phy/Makefile                               |    3 +
> >  drivers/phy/phy-exynos4210-usb2.c                  |  264
> +++++++++++++++++
> >  drivers/phy/phy-exynos4212-usb2.c                  |  312
> ++++++++++++++++++++
> >  drivers/phy/phy-samsung-usb2.c                     |  228
> ++++++++++++++
> >  drivers/phy/phy-samsung-usb2.h                     |   72 +++++
> >  7 files changed, 953 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> >  create mode 100644 drivers/phy/phy-exynos4210-usb2.c  create mode
> > 100644 drivers/phy/phy-exynos4212-usb2.c  create mode 100644
> > drivers/phy/phy-samsung-usb2.c  create mode 100644
> > drivers/phy/phy-samsung-usb2.h
> >
> > diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > new file mode 100644
> > index 0000000..cadbf70
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> 
> use the existing samsung-phy.txt.

Ok.

> > @@ -0,0 +1,54 @@
> > +Samsung S5P/EXYNOS SoC series USB PHY
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : should be one of the listed compatibles:
> > +	- "samsung,exynos4210-usb2-phy"
> > +	- "samsung,exynos4212-usb2-phy"
> > +- reg : a list of registers used by phy driver
> > +	- first and obligatory is the location of phy modules registers
> > +- samsung,sysreg-phandle - handle to syscon used to control the
> > +system registers
> > +- samsung,pmureg-phandle - handle to syscon used to control PMU
> > +registers
> > +- #phy-cells : from the generic phy bindings, must be 1;
> > +- clocks and clock-names:
> > +	- the "phy" clocks is required by the phy module
> > +	- next for each of the phys a clock has to be assidned, this
> clock
> 
> %s/assidned/assigned/

Thank you for spotting this.

> > +	  will be used to determine clocking frequency for the phys
> > +	  (the labels are specified in the paragraph below)
> > +
> > +The first phandle argument in the PHY specifier identifies the PHY,
> > +its meaning is compatible dependent. For the currently supported
> SoCs
> > +(Exynos 4210 and Exynos 4212) it is as follows:
> > +  0 - USB device ("device"),
> > +  1 - USB host ("host"),
> > +  2 - HSIC0 ("hsic0"),
> > +  3 - HSIC1 ("hsic1"),
> > +
> > +Exynos 4210 and Exynos 4212 use mode switching and require that mode
> > +switch register is supplied.
> > +
> > +Example:
> > +
> > +For Exynos 4412 (compatible with Exynos 4212):
> > +
> > +usbphy: phy@125B0000 {
> 
> use lower case for address here...

Ok.

> > +	compatible = "samsung,exynos4212-usb2-phy";
> > +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> and here..
> > +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> > +							<&clock 2>;
> > +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> > +	status = "okay";
> > +	#phy-cells = <1>;
> > +	samsung,sysreg-phandle = <&sys_reg>;
> > +	samsung,pmureg-phandle = <&pmu_reg>; };
> > +
> > +Then the PHY can be used in other nodes such as:
> > +
> > +phy-consumer@12340000 {
> > +	phys = <&usbphy 2>;
> > +	phy-names = "phy";
> > +};
> > +
> > +Refer to DT bindings documentation of particular PHY consumer
> devices
> > +for more information about required PHYs and the way of
> specification.
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
> > a344f3d..b29018f 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
> >  	help
> >  	  Support for Display Port PHY found on Samsung EXYNOS SoCs.
> >
> > +config PHY_SAMSUNG_USB2
> > +	tristate "Samsung USB 2.0 PHY driver"
> > +	help
> > +	  Enable this to support Samsung USB phy helper driver for
> Samsung SoCs.
> > +	  This driver provides common interface to interact, for Samsung
> > +	  USB 2.0 PHY driver.
> > +
> > +config PHY_EXYNOS4210_USB2
> > +	bool "Support for Exynos 4210"
> > +	depends on PHY_SAMSUNG_USB2
> > +	depends on CPU_EXYNOS4210
> 
> select GENERIC_PHY here?

I think that depends on PHY_SAMSUNG_USB2 is better in this place.
However, I agree that I should add select GENERIC_PHY to PHY_SAMSUNG_USB2.

The reason why I am saying this is that I like how it looks in
menuconfig. Selecting PHY_SAMSUNG_USB2 expands more options and if
unselected the menu looks tidier.

> > +	help
> > +	  Enable USB PHY support for Exynos 4210
> 
> Add more explanation here and make checkpatch happy.

Here I think we should not treat checkpatch as an oracle. I also noticed
these warnings, but I think that writing a four line description for
SoC specific options in this menu is an overkill. In addition, checkpatch
also complains for the following entries in Kconfig file:
- PHY_EXYNOS_MIPI_VIDEO
- PHY_EXYNOS_DP_VIDEO
- PHY_SAMSUNG_USB2 (here expanding the description may be justified, but
  I think the current description is enough)

But thank you for directing my attention to the Kconfig file. I noticed
That SoC specific Kconfig entries should have "USB 2.0" instead of just
"USB".

> > +
> > +config PHY_EXYNOS4212_USB2
> > +	bool "Support for Exynos 4212"
> > +	depends on PHY_SAMSUNG_USB2
> > +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> 
> select GENERIC_PHY.

Please check my reply above.

> > +	help
> > +	  Enable USB PHY support for Exynos 4212
> 
> more explanation here too..

Please check my reply above.

> >  endmenu
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
> > d0caae9..9f4befd 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
> > +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
> > +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
> > diff --git a/drivers/phy/phy-exynos4210-usb2.c
> > b/drivers/phy/phy-exynos4210-usb2.c
> > new file mode 100644
> > index 0000000..a02e5c2
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos4210-usb2.c
> > @@ -0,0 +1,264 @@
> > +/*
> > + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@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/regmap.h>
> > +#include <linux/spinlock.h>
> 
> You've included most of the above header files in phy-samsung-usb2.h
> which you are including below.

I agree that includes in phy-samsung-usb2.h could use a cleanup. On the
other
hand my opinion is that a .c file should include all .h files that are used
in
this .c file. Relaying on .h file to include another .h doesn't seem good to
me.

> > +#include "phy-samsung-usb2.h"
> > +
> > +/* Exynos USB PHY registers */
> > +
> > +/* PHY power control */
> > +#define EXYNOS_4210_UPHYPWR			0x0
> > +
> > +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> 
> use BIT() here and everywhere below.

All right.

> > +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)
> 
> replace PHY0 here with DEV so it looks similar to EXYNOS_4212.
> > +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> > +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> > +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> > +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> > +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> > +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> > +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> > +
> > +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)
> 
> replace PHY0 here with HOST so it looks similar to EXYNOS_4212.

Thank you for spotting this inconsistence. I will fix that. But I
rather incline to use PHY0/1 instead of DEVICE/HOST as it will be
then consistent with the date sheet.

> > +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> > +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> > +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> > +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> > +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> > +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> > +
> > +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> > +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> > +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> > +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> > +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> > +
> > +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> > +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> > +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> > +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> > +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> > +
> > +/* PHY clock control */
> > +#define EXYNOS_4210_UPHYCLK			0x4
> > +
> > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> > +
> > +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> > +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> > +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> > +
> > +/* PHY reset control */
> > +#define EXYNOS_4210_UPHYRST			0x8
> > +
> > +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> > +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> > +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> > +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> > +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> > +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> > +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> > +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> > +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> > +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)
> > +
> > +/* Isolation, configured in the power management unit */
> > +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x704
> > +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> > +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x708
> > +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> > +
> > +/* USBYPHY1 Floating prevention */
> > +#define EXYNOS_4210_UPHY1CON			0x34
> > +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> > +
> > +/* Mode switching SUB Device <-> Host */
> > +#define EXYNOS_4210_MODE_SWITCH_OFFSET		0x21c
> > +#define EXYNOS_4210_MODE_SWITCH_MASK		1
> > +#define EXYNOS_4210_MODE_SWITCH_DEVICE		0
> > +#define EXYNOS_4210_MODE_SWITCH_HOST		1
> > +
> > +enum exynos4210_phy_id {
> > +	EXYNOS4210_DEVICE,
> > +	EXYNOS4210_HOST,
> > +	EXYNOS4210_HSIC0,
> > +	EXYNOS4210_HSIC1,
> > +	EXYNOS4210_NUM_PHYS,
> > +};
> > +
> > +/*
> > + * exynos4210_rate_to_clk() converts the supplied clock rate to the
> > +value that
> > + * can be written to the phy register.
> > + */
> > +static int exynos4210_rate_to_clk(unsigned long rate, u32 *reg) {
> > +	switch (rate) {
> > +	case 12 * MHZ:
> > +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> > +		break;
> > +	case 24 * MHZ:
> > +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> > +		break;
> > +	case 48 * MHZ:
> > +		reg = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> 
> %s/reg/*Reg

Thank you.

> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void exynos4210_isol(struct samsung_usb2_phy_instance *inst,
> > +bool on) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	u32 offset;
> > +	u32 mask;
> > +
> > +	switch (inst->cfg->id) {
> > +	case EXYNOS4210_DEVICE:
> > +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> > +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> > +		break;
> > +	case EXYNOS4210_HOST:
> > +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> > +		mask = EXYNOS_4210_USB_ISOL_HOST;
> > +		break;
> > +	default:
> > +		return;
> > +	};
> > +
> > +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); }
> > +
> > +static void exynos4210_phy_pwr(struct samsung_usb2_phy_instance
> > +*inst, bool on) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	u32 rstbits = 0;
> > +	u32 phypwr = 0;
> > +	u32 rst;
> > +	u32 pwr;
> > +
> > +	switch (inst->cfg->id) {
> > +	case EXYNOS4210_DEVICE:
> > +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> > +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> > +		break;
> > +	case EXYNOS4210_HOST:
> > +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> > +				EXYNOS_4210_URSTCON_PHY1_P0 |
> > +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> > +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> > +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> > +		break;
> > +	case EXYNOS4210_HSIC0:
> > +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> > +		break;
> > +	case EXYNOS4210_HSIC1:
> > +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> > +		break;
> > +	};
> > +
> > +	if (on) {
> > +		writel(inst->clk_reg_val, drv->reg_phy +
> EXYNOS_4210_UPHYCLK);
> > +
> > +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > +		pwr &= ~phypwr;
> > +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > +
> > +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> > +		rst |= rstbits;
> > +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> > +		udelay(10);
> > +		rst &= ~rstbits;
> > +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> 
> Do you have to reset during every power on? IMO this reset should be
> done once in phy_init.

It seems that this reset is neded by the hardware.

> > +	} else {
> > +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > +		pwr |= phypwr;
> > +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > +	}
> > +}
> > +
> > +static int exynos4210_power_on(struct samsung_usb2_phy_instance
> > +*inst) {
> > +	/* Order of initialisation is important - first power then
> isolation */
> > +	exynos4210_phy_pwr(inst, 1);
> > +	exynos4210_isol(inst, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos4210_power_off(struct samsung_usb2_phy_instance
> > +*inst) {
> > +	exynos4210_isol(inst, 1);
> > +	exynos4210_phy_pwr(inst, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static const struct samsung_usb2_common_phy exynos4210_phys[] = {
> > +	{
> > +		.label		= "device",
> > +		.id		= EXYNOS4210_DEVICE,
> > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > +		.power_on	= exynos4210_power_on,
> > +		.power_off	= exynos4210_power_off,
> > +	},
> > +	{
> > +		.label		= "host",
> > +		.id		= EXYNOS4210_HOST,
> > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > +		.power_on	= exynos4210_power_on,
> > +		.power_off	= exynos4210_power_off,
> > +	},
> > +	{
> > +		.label		= "hsic0",
> > +		.id		= EXYNOS4210_HSIC0,
> > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > +		.power_on	= exynos4210_power_on,
> > +		.power_off	= exynos4210_power_off,
> > +	},
> > +	{
> > +		.label		= "hsic1",
> > +		.id		= EXYNOS4210_HSIC1,
> > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > +		.power_on	= exynos4210_power_on,
> > +		.power_off	= exynos4210_power_off,
> > +	},
> > +	{},
> > +};
> > +
> > +const struct samsung_usb2_phy_config exynos4210_usb2_phy_config = {
> > +	.num_phys		= EXYNOS4210_NUM_PHYS,
> > +	.phys			= exynos4210_phys,
> > +	.has_mode_switch	= 1,
> > +};
> > +
> > diff --git a/drivers/phy/phy-exynos4212-usb2.c
> > b/drivers/phy/phy-exynos4212-usb2.c
> > new file mode 100644
> > index 0000000..375ece0
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos4212-usb2.c
> > @@ -0,0 +1,312 @@
> > +/*
> > + * Samsung S5P/EXYNOS SoC series USB PHY driver - Exynos 4212
> support
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@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/regmap.h>
> > +#include <linux/spinlock.h>
> 
> You've included most of the above header files in phy-samsung-usb2.h
> which you are including below.

Please see my reply above.

> > +#include "phy-samsung-usb2.h"
> > +
> > +/* Exynos USB PHY registers */
> > +
> > +/* PHY power control */
> > +#define EXYNOS_4212_UPHYPWR			0x0
> > +
> > +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)
> 
> use BIT() here and below..

All right.

> > +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> > +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> > +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> > +#define EXYNOS_4212_UPHYPWR_DEV	( \
> > +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> > +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> > +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> > +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> > +
> > +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> > +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> > +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> > +#define EXYNOS_4212_UPHYPWR_HOST ( \
> > +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> > +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> > +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> > +
> > +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> > +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> > +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> > +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> > +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> > +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> > +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> > +
> > +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> > +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> > +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> > +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> > +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> > +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> > +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> > +
> > +/* PHY clock control */
> > +#define EXYNOS_4212_UPHYCLK			0x4
> > +
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> > +
> > +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> > +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> > +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> > +
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> > +
> > +/* PHY reset control */
> > +#define EXYNOS_4212_UPHYRST			0x8
> > +
> > +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> > +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> > +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> > +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> > +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> > +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> > +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> > +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> > +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> > +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> > +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> > +
> > +/* Isolation, configured in the power management unit */
> > +#define EXYNOS_4212_USB_ISOL_OFFSET		0x704
> > +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> > +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x708
> > +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> > +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x70c
> > +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> > +
> > +/* Mode switching SUB Device <-> Host */
> > +#define EXYNOS_4212_MODE_SWITCH_OFFSET		0x21c
> > +#define EXYNOS_4212_MODE_SWITCH_MASK		1
> > +#define EXYNOS_4212_MODE_SWITCH_DEVICE		0
> > +#define EXYNOS_4212_MODE_SWITCH_HOST		1
> > +
> > +enum exynos4x12_phy_id {
> > +	EXYNOS4212_DEVICE,
> > +	EXYNOS4212_HOST,
> > +	EXYNOS4212_HSIC0,
> > +	EXYNOS4212_HSIC1,
> > +	EXYNOS4212_NUM_PHYS,
> > +};
> > +
> > +/*
> > + * exynos4212_rate_to_clk() converts the supplied clock rate to the
> > +value that
> > + * can be written to the phy register.
> > + */
> > +static int exynos4212_rate_to_clk(unsigned long rate, u32 *reg) {
> > +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> > +
> > +	switch (rate) {
> > +	case 9600 * KHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> > +		break;
> > +	case 10 * MHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> > +		break;
> > +	case 12 * MHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> > +		break;
> > +	case 19200 * KHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> > +		break;
> > +	case 20 * MHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> > +		break;
> > +	case 24 * MHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> > +		break;
> > +	case 50 * MHZ:
> > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void exynos4212_isol(struct samsung_usb2_phy_instance *inst,
> > +bool on) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	u32 offset;
> > +	u32 mask;
> > +
> > +	switch (inst->cfg->id) {
> > +	case EXYNOS4212_DEVICE:
> > +	case EXYNOS4212_HOST:
> > +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> > +		mask = EXYNOS_4212_USB_ISOL_OTG;
> > +		break;
> > +	case EXYNOS4212_HSIC0:
> > +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> > +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> > +		break;
> > +	case EXYNOS4212_HSIC1:
> > +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> > +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> > +		break;
> > +	default:
> > +		return;
> > +	};
> > +
> > +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); }
> > +
> > +static void exynos4212_phy_pwr(struct samsung_usb2_phy_instance
> > +*inst, bool on) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	u32 rstbits = 0;
> > +	u32 phypwr = 0;
> > +	u32 rst;
> > +	u32 pwr;
> > +
> > +	switch (inst->cfg->id) {
> > +	case EXYNOS4212_DEVICE:
> > +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> > +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> > +		break;
> > +	case EXYNOS4212_HOST:
> > +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> > +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> > +		break;
> > +	case EXYNOS4212_HSIC0:
> > +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> > +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> > +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> > +				EXYNOS_4212_URSTCON_HOST_PHY;
> > +		break;
> > +	case EXYNOS4212_HSIC1:
> > +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> > +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> > +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> > +		break;
> > +	};
> > +
> > +	if (on) {
> > +		writel(inst->clk_reg_val, drv->reg_phy +
> EXYNOS_4212_UPHYCLK);
> > +
> > +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > +		pwr &= ~phypwr;
> > +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > +
> > +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> > +		rst |= rstbits;
> > +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> > +		udelay(10);
> > +		rst &= ~rstbits;
> > +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> 
> reset should be done once in init?

The hardware seems to need this reset.

> > +	} else {
> > +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > +		pwr |= phypwr;
> > +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > +	}
> > +}
> > +
> > +static int exynos4212_power_on(struct samsung_usb2_phy_instance
> > +*inst) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +
> > +	inst->enabled = 1;
> > +	exynos4212_phy_pwr(inst, 1);
> > +	exynos4212_isol(inst, 0);
> > +
> > +	/* Power on the device, as it is necessary for HSIC to work */
> 
> This looks weird. How powering on the 'device PHY' makes 'HSIC PHY' to
> work?

I agree with you that it looks weird, but I found this necessary to be done.

> > +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> > +		struct samsung_usb2_phy_instance *device =
> > +					&drv->instances[EXYNOS4212_DEVICE];
> > +		exynos4212_phy_pwr(device, 1);
> > +		exynos4212_isol(device, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos4212_power_off(struct samsung_usb2_phy_instance
> > +*inst) {
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	struct samsung_usb2_phy_instance *device =
> > +&drv->instances[EXYNOS4212_DEVICE];
> > +
> > +	inst->enabled = 0;
> > +	exynos4212_isol(inst, 1);
> > +	exynos4212_phy_pwr(inst, 0);
> > +
> > +	if (inst->cfg->id == EXYNOS4212_HSIC0 && !device->enabled) {
> > +		exynos4212_isol(device, 1);
> > +		exynos4212_phy_pwr(device, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static const struct samsung_usb2_common_phy exynos4212_phys[] = {
> > +	{
> > +		.label		= "device",
> > +		.id		= EXYNOS4212_DEVICE,
> > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > +		.power_on	= exynos4212_power_on,
> > +		.power_off	= exynos4212_power_off,
> > +	},
> > +	{
> > +		.label		= "host",
> > +		.id		= EXYNOS4212_HOST,
> > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > +		.power_on	= exynos4212_power_on,
> > +		.power_off	= exynos4212_power_off,
> > +	},
> > +	{
> > +		.label		= "hsic0",
> > +		.id		= EXYNOS4212_HSIC0,
> > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > +		.power_on	= exynos4212_power_on,
> > +		.power_off	= exynos4212_power_off,
> > +	},
> > +	{
> > +		.label		= "hsic1",
> > +		.id		= EXYNOS4212_HSIC1,
> > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > +		.power_on	= exynos4212_power_on,
> > +		.power_off	= exynos4212_power_off,
> > +	},
> > +	{},
> > +};
> > +
> > +const struct samsung_usb2_phy_config exynos4212_usb2_phy_config = {
> > +	.num_phys		= EXYNOS4212_NUM_PHYS,
> > +	.phys			= exynos4212_phys,
> > +	.has_mode_switch	= 1,
> > +};
> > +
> > diff --git a/drivers/phy/phy-samsung-usb2.c
> > b/drivers/phy/phy-samsung-usb2.c new file mode 100644 index
> > 0000000..804ec77
> > --- /dev/null
> > +++ b/drivers/phy/phy-samsung-usb2.c
> > @@ -0,0 +1,228 @@
> > +/*
> > + * Samsung SoC USB 1.1/2.0 PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@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/mfd/syscon.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/spinlock.h>
> 
> You've included most of the above header files in phy-samsung-usb2.h
> which you are including below.

Please see my answer above.

> > +#include "phy-samsung-usb2.h"
> > +
> > +static int samsung_usb2_phy_power_on(struct phy *phy) {
> > +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	int ret;
> > +
> > +	dev_dbg(drv->dev, "Request to power_on \"%s\" usb phy\n",
> > +							inst->cfg->label);
> > +	ret = clk_prepare_enable(drv->clk);
> > +	if (ret)
> > +		goto err_main_clk;
> > +	ret = clk_prepare_enable(inst->clk);
> > +	if (ret)
> > +		goto err_instance_clk;
> > +	inst->rate = clk_get_rate(inst->clk);
> > +	if (inst->cfg->rate_to_clk) {
> > +		ret = inst->cfg->rate_to_clk(inst->rate, &inst-
> >clk_reg_val);
> > +		if (ret)
> > +			goto err_get_rate;
> > +	}
> > +	if (inst->cfg->power_on) {
> > +		spin_lock(&drv->lock);
> > +		ret = inst->cfg->power_on(inst);
> > +		spin_unlock(&drv->lock);
> > +	}
> > +
> > +	return 0;
> > +
> > +err_get_rate:
> > +	clk_disable_unprepare(inst->clk);
> > +err_instance_clk:
> > +	clk_disable_unprepare(drv->clk);
> > +err_main_clk:
> > +	return ret;
> > +}
> > +
> > +static int samsung_usb2_phy_power_off(struct phy *phy) {
> > +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > +	int ret = 0;
> > +
> > +	dev_dbg(drv->dev, "Request to power_off \"%s\" usb phy\n",
> > +							inst->cfg->label);
> > +	if (inst->cfg->power_off) {
> > +		spin_lock(&drv->lock);
> > +		ret = inst->cfg->power_off(inst);
> > +		spin_unlock(&drv->lock);
> > +	}
> > +	clk_disable_unprepare(inst->clk);
> > +	clk_disable_unprepare(drv->clk);
> > +	return ret;
> > +}
> > +
> > +static struct phy_ops samsung_usb2_phy_ops = {
> > +	.power_on	= samsung_usb2_phy_power_on,
> > +	.power_off	= samsung_usb2_phy_power_off,
> > +	.owner		= THIS_MODULE,
> > +};
> > +
> > +static struct phy *samsung_usb2_phy_xlate(struct device *dev,
> > +					struct of_phandle_args *args)
> > +{
> > +	struct samsung_usb2_phy_driver *drv;
> > +
> > +	drv = dev_get_drvdata(dev);
> > +	if (!drv)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	return drv->instances[args->args[0]].phy;
> > +}
> > +
> > +static const struct of_device_id samsung_usb2_phy_of_match[] = {
> > +#ifdef CONFIG_PHY_EXYNOS4210_USB2
> > +	{
> > +		.compatible = "samsung,exynos4210-usb2-phy",
> > +		.data = &exynos4210_usb2_phy_config,
> > +	},
> > +#endif
> > +#ifdef CONFIG_PHY_EXYNOS4212_USB2
> > +	{
> > +		.compatible = "samsung,exynos4212-usb2-phy",
> > +		.data = &exynos4212_usb2_phy_config,
> > +	},
> > +#endif
> > +	{ },
> > +};
> 
> I think we've had enough discussion about this approach. Let's get the
> opinion of others too. Felipe? Greg?

Good idea.

> Summary:
> We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
> almost similar register map [1] and a samsung helper driver for these
> two drivers.

I would not call them separate drivers. It's a single USB 2.0 driver with
the option to include support for various SoCs. This patchset adds:
Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another
person is working on supporting S3C6410.

> These two PHY drivers populate the function pointers in the helper
> driver. So any phy_ops will first invoke the helper driver which will
> then invoke the corresponding phy driver.
> 
> [1] -> http://www.diffchecker.com/7yno1uvk

Come on, this diff only includes the registers part of the file. 
The following functions are also different:
- exynos421*_rate_to_clk
- exynos421*_isol
- exynos421*_phy_pwr
- exynos421*_power_on
- exynos421*_power_on

It seems that the file is too large for the tool. But still this makes a
false impression that only registers are different.
 
> Advantages:
> * (more) clean and readable
> * helper driver can be used with other PHY drivers which will be added
> soon
> 
> Disadvantages:
> * code duplication

I would say that actually in this case less code is duplicated. Having
Separate drivers would mean that most of the phy-samsung-usb2.c file has
to be repeated. That is 240 times 4 (and surely more in the future, as
this patchset adds support for 4 SoCs). Which is almost 1000 lines more.

> 
> Maybe having a helper driver makes sense when we have other samsung PHY
> drivers added but not sure if it's needed here for EXYNOS4210_USB2 and
> EXYNOS4212_USB2
> 
> Need your inputs on what you think about this.

Yes, I would also welcome other people's opinions. 

> > +
> > +static int samsung_usb2_phy_probe(struct platform_device *pdev) {
> > +	const struct of_device_id *match;
> > +	const struct samsung_usb2_phy_config *cfg;
> > +	struct clk *clk;
> > +	struct device *dev = &pdev->dev;
> > +	struct phy_provider *phy_provider;
> > +	struct resource *mem;
> > +	struct samsung_usb2_phy_driver *drv;
> > +	int i;
> > +
> > +	if (!pdev->dev.of_node) {
> > +		dev_err(dev, "This driver is required to be instantiated
> from device tree\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	match = of_match_node(samsung_usb2_phy_of_match, pdev-
> >dev.of_node);
> > +	if (!match) {
> > +		dev_err(dev, "of_match_node() failed\n");
> > +		return -EINVAL;
> > +	}
> > +	cfg = match->data;
> > +
> > +	drv = devm_kzalloc(dev, sizeof(struct samsung_usb2_phy_driver) +
> > +		cfg->num_phys * sizeof(struct samsung_usb2_phy_instance),
> GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, drv);
> > +	spin_lock_init(&drv->lock);
> > +
> > +	drv->cfg = cfg;
> > +	drv->dev = dev;
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> > +	if (IS_ERR(drv->reg_phy)) {
> > +		dev_err(dev, "Failed to map register memory (phy)\n");
> > +		return PTR_ERR(drv->reg_phy);
> > +	}
> > +
> > +	drv->reg_pmu = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > +		"samsung,pmureg-phandle");
> > +	if (IS_ERR(drv->reg_pmu)) {
> > +		dev_err(dev, "Failed to map PMU registers (via syscon)\n");
> > +		return PTR_ERR(drv->reg_pmu);
> > +	}
> > +
> > +	if (drv->cfg->has_mode_switch) {
> > +		drv->reg_sys = syscon_regmap_lookup_by_phandle(
> > +				pdev->dev.of_node,
"samsung,sysreg-phandle");
> > +		if (IS_ERR(drv->reg_sys)) {
> > +			dev_err(dev, "Failed to map system registers (via
> syscon)\n");
> > +			return PTR_ERR(drv->reg_sys);
> > +		}
> > +	}
> > +
> > +	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);
> > +	}
> > +
> > +	for (i = 0; i < drv->cfg->num_phys; i++) {
> > +		char *label = drv->cfg->phys[i].label;
> > +		struct samsung_usb2_phy_instance *p = &drv->instances[i];
> > +
> > +		dev_dbg(dev, "Creating phy \"%s\"\n", label);
> > +		p->phy = devm_phy_create(dev, &samsung_usb2_phy_ops, NULL);
> > +		if (IS_ERR(p->phy)) {
> > +			dev_err(drv->dev, "Failed to create usb2_phy
> \"%s\"\n",
> > +						label);
> > +			return PTR_ERR(p->phy);
> > +		}
> > +
> > +		p->cfg = &drv->cfg->phys[i];
> > +		p->drv = drv;
> > +		phy_set_drvdata(p->phy, p);
> > +
> > +		clk = devm_clk_get(dev, p->cfg->label);
> > +		if (IS_ERR(clk)) {
> > +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> > +
p->cfg->label);
> > +			return PTR_ERR(clk);
> > +		}
> > +		p->clk = clk;
> > +	}
> > +
> > +	phy_provider = devm_of_phy_provider_register(dev,
> > +
samsung_usb2_phy_xlate);
> > +	if (IS_ERR(phy_provider)) {
> > +		dev_err(drv->dev, "Failed to register phy provider\n");
> > +		return PTR_ERR(phy_provider);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver samsung_usb2_phy_driver = {
> > +	.probe	= samsung_usb2_phy_probe,
> > +	.driver = {
> > +		.of_match_table	= samsung_usb2_phy_of_match,
> > +		.name		= "samsung-usb2-phy",
> > +		.owner		= THIS_MODULE,
> > +	}
> > +};
> > +
> > +module_platform_driver(samsung_usb2_phy_driver);
> > +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> > +MODULE_AUTHOR("Kamil Debski <k.debski@samsung.com>");
> > +MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:samsung-usb2-phy");
> > +
> > diff --git a/drivers/phy/phy-samsung-usb2.h
> > b/drivers/phy/phy-samsung-usb2.h new file mode 100644 index
> > 0000000..cd12477
> > --- /dev/null
> > +++ b/drivers/phy/phy-samsung-usb2.h
> > @@ -0,0 +1,72 @@
> > +/*
> > + * Samsung SoC USB 1.1/2.0 PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@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.
> > + */
> > +
> > +#ifndef _PHY_EXYNOS_USB2_H
> > +#define _PHY_EXYNOS_USB2_H
> > +
> > +#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/spinlock.h>
> > +
> > +#define KHZ 1000
> > +#define MHZ (KHZ * KHZ)
> > +
> > +struct samsung_usb2_phy_driver;
> > +struct samsung_usb2_phy_instance;
> > +struct samsung_usb2_phy_config;
> > +
> > +struct samsung_usb2_phy_instance {
> > +	struct samsung_usb2_phy_driver *drv;
> > +	struct phy *phy;
> > +	const struct samsung_usb2_common_phy *cfg;
> > +	char enabled;
> > +	struct clk *clk;
> > +	u32 clk_reg_val;
> > +	unsigned long rate;
> > +};
> > +
> > +struct samsung_usb2_phy_driver {
> > +	struct device *dev;
> > +	spinlock_t lock;
> > +	void __iomem *reg_phy;
> > +	struct regmap *reg_sys;
> > +	struct regmap *reg_pmu;
> > +	const struct samsung_usb2_phy_config *cfg;
> > +	struct clk *clk;
> > +	struct samsung_usb2_phy_instance instances[0]; };
> > +
> > +struct samsung_usb2_common_phy {
> > +	char *label;
> > +	unsigned int id;
> > +	int (*rate_to_clk)(unsigned long, u32 *);
> > +	int (*power_on)(struct samsung_usb2_phy_instance *);
> > +	int (*power_off)(struct samsung_usb2_phy_instance *); };
> > +
> > +
> > +struct samsung_usb2_phy_config {
> > +	int num_phys;
> > +	const struct samsung_usb2_common_phy *phys;
> > +	char has_mode_switch;
> 
> u8 instead?

Do we really need to specify that we need 8bits? Why do you think
char is wrong?

Please read this paragraph from LDD3:
"Sometimes kernel code requires data items of a specific size,
perhaps to match predefined binary structures,* to communicate with
user space, or to align data within structures by inserting
"padding" fields (but refer to the section "Data Alignment" for
information about alignment issues)."
Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf

has_mode_switch is only a flag 0 or 1. Never written anywhere in
hardware registers. Used in an if somewhere in code. Give me a good
reason why do you think it should be explicitly 8 bit long.

Best wishes,

Hi,

> From: Kamil Debski [mailto:k.debski@samsung.com]
> Sent: Friday, December 06, 2013 5:28 PM
> 
> Hi Kishon,
> 
> Thank you for the review.
> 
> > From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> > Sent: Friday, December 06, 2013 11:59 AM
> >
> > Hi,
> >
> > On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
> > > Add a new driver for the Exynos USB PHY. The new driver uses the
> > > generic PHY framework. The driver includes support for the Exynos
> > 4x10
> > > and 4x12 SoC families.
> > >
> > > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > ---
> > >  .../devicetree/bindings/phy/samsung-usbphy.txt     |   54 ++++
> > >  drivers/phy/Kconfig                                |   20 ++
> > >  drivers/phy/Makefile                               |    3 +
> > >  drivers/phy/phy-exynos4210-usb2.c                  |  264
> > +++++++++++++++++
> > >  drivers/phy/phy-exynos4212-usb2.c                  |  312
> > ++++++++++++++++++++
> > >  drivers/phy/phy-samsung-usb2.c                     |  228
> > ++++++++++++++
> > >  drivers/phy/phy-samsung-usb2.h                     |   72 +++++
> > >  7 files changed, 953 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > >  create mode 100644 drivers/phy/phy-exynos4210-usb2.c  create mode
> > > 100644 drivers/phy/phy-exynos4212-usb2.c  create mode 100644
> > > drivers/phy/phy-samsung-usb2.c  create mode 100644
> > > drivers/phy/phy-samsung-usb2.h
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > > b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > > new file mode 100644
> > > index 0000000..cadbf70
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> >
> > use the existing samsung-phy.txt.
> 
> Ok.
> 
> > > @@ -0,0 +1,54 @@
> > > +Samsung S5P/EXYNOS SoC series USB PHY
> > > +-------------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible : should be one of the listed compatibles:
> > > +	- "samsung,exynos4210-usb2-phy"
> > > +	- "samsung,exynos4212-usb2-phy"
> > > +- reg : a list of registers used by phy driver
> > > +	- first and obligatory is the location of phy modules registers
> > > +- samsung,sysreg-phandle - handle to syscon used to control the
> > > +system registers
> > > +- samsung,pmureg-phandle - handle to syscon used to control PMU
> > > +registers
> > > +- #phy-cells : from the generic phy bindings, must be 1;
> > > +- clocks and clock-names:
> > > +	- the "phy" clocks is required by the phy module
> > > +	- next for each of the phys a clock has to be assidned, this
> > clock
> >
> > %s/assidned/assigned/
> 
> Thank you for spotting this.
> 
> > > +	  will be used to determine clocking frequency for the phys
> > > +	  (the labels are specified in the paragraph below)
> > > +
> > > +The first phandle argument in the PHY specifier identifies the PHY,
> > > +its meaning is compatible dependent. For the currently supported
> > SoCs
> > > +(Exynos 4210 and Exynos 4212) it is as follows:
> > > +  0 - USB device ("device"),
> > > +  1 - USB host ("host"),
> > > +  2 - HSIC0 ("hsic0"),
> > > +  3 - HSIC1 ("hsic1"),
> > > +
> > > +Exynos 4210 and Exynos 4212 use mode switching and require that
> > > +mode switch register is supplied.
> > > +
> > > +Example:
> > > +
> > > +For Exynos 4412 (compatible with Exynos 4212):
> > > +
> > > +usbphy: phy@125B0000 {
> >
> > use lower case for address here...
> 
> Ok.
> 
> > > +	compatible = "samsung,exynos4212-usb2-phy";
> > > +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> > and here..
> > > +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> > > +							<&clock 2>;
> > > +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> > > +	status = "okay";
> > > +	#phy-cells = <1>;
> > > +	samsung,sysreg-phandle = <&sys_reg>;
> > > +	samsung,pmureg-phandle = <&pmu_reg>; };
> > > +
> > > +Then the PHY can be used in other nodes such as:
> > > +
> > > +phy-consumer@12340000 {
> > > +	phys = <&usbphy 2>;
> > > +	phy-names = "phy";
> > > +};
> > > +
> > > +Refer to DT bindings documentation of particular PHY consumer
> > devices
> > > +for more information about required PHYs and the way of
> > specification.
> > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
> > > a344f3d..b29018f 100644
> > > --- a/drivers/phy/Kconfig
> > > +++ b/drivers/phy/Kconfig
> > > @@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
> > >  	help
> > >  	  Support for Display Port PHY found on Samsung EXYNOS SoCs.
> > >
> > > +config PHY_SAMSUNG_USB2
> > > +	tristate "Samsung USB 2.0 PHY driver"
> > > +	help
> > > +	  Enable this to support Samsung USB phy helper driver for
> > Samsung SoCs.
> > > +	  This driver provides common interface to interact, for Samsung
> > > +	  USB 2.0 PHY driver.
> > > +
> > > +config PHY_EXYNOS4210_USB2
> > > +	bool "Support for Exynos 4210"
> > > +	depends on PHY_SAMSUNG_USB2
> > > +	depends on CPU_EXYNOS4210
> >
> > select GENERIC_PHY here?
> 
> I think that depends on PHY_SAMSUNG_USB2 is better in this place.
> However, I agree that I should add select GENERIC_PHY to
> PHY_SAMSUNG_USB2.
> 
> The reason why I am saying this is that I like how it looks in
> menuconfig. Selecting PHY_SAMSUNG_USB2 expands more options and if
> unselected the menu looks tidier.
> 
> > > +	help
> > > +	  Enable USB PHY support for Exynos 4210
> >
> > Add more explanation here and make checkpatch happy.
> 
> Here I think we should not treat checkpatch as an oracle. I also
> noticed these warnings, but I think that writing a four line
> description for SoC specific options in this menu is an overkill. In
> addition, checkpatch also complains for the following entries in
> Kconfig file:
> - PHY_EXYNOS_MIPI_VIDEO
> - PHY_EXYNOS_DP_VIDEO
> - PHY_SAMSUNG_USB2 (here expanding the description may be justified,
> but
>   I think the current description is enough)
> 
> But thank you for directing my attention to the Kconfig file. I noticed
> That SoC specific Kconfig entries should have "USB 2.0" instead of just
> "USB".
> 
> > > +
> > > +config PHY_EXYNOS4212_USB2
> > > +	bool "Support for Exynos 4212"
> > > +	depends on PHY_SAMSUNG_USB2
> > > +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> >
> > select GENERIC_PHY.
> 
> Please check my reply above.
> 
> > > +	help
> > > +	  Enable USB PHY support for Exynos 4212
> >
> > more explanation here too..
> 
> Please check my reply above.
> 
> > >  endmenu
> > > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
> > > d0caae9..9f4befd 100644
> > > --- a/drivers/phy/Makefile
> > > +++ b/drivers/phy/Makefile
> > > @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
> > > +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
> > > +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
> > > diff --git a/drivers/phy/phy-exynos4210-usb2.c
> > > b/drivers/phy/phy-exynos4210-usb2.c
> > > new file mode 100644
> > > index 0000000..a02e5c2
> > > --- /dev/null
> > > +++ b/drivers/phy/phy-exynos4210-usb2.c
> > > @@ -0,0 +1,264 @@
> > > +/*
> > > + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
> > > + *
> > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Kamil Debski <k.debski@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/regmap.h>
> > > +#include <linux/spinlock.h>
> >
> > You've included most of the above header files in phy-samsung-usb2.h
> > which you are including below.
> 
> I agree that includes in phy-samsung-usb2.h could use a cleanup. On the
> other hand my opinion is that a .c file should include all .h files
> that are used in this .c file. Relaying on .h file to include
> another .h doesn't seem good to me.
> 
> > > +#include "phy-samsung-usb2.h"
> > > +
> > > +/* Exynos USB PHY registers */
> > > +
> > > +/* PHY power control */
> > > +#define EXYNOS_4210_UPHYPWR			0x0
> > > +
> > > +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> >
> > use BIT() here and everywhere below.
> 
> All right.
> 
> > > +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)
> >
> > replace PHY0 here with DEV so it looks similar to EXYNOS_4212.
> > > +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> > > +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> > > +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> > > +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> > > +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> > > +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> > > +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> > > +
> > > +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)
> >
> > replace PHY0 here with HOST so it looks similar to EXYNOS_4212.
> 
> Thank you for spotting this inconsistence. I will fix that. But I
> rather incline to use PHY0/1 instead of DEVICE/HOST as it will be then
> consistent with the date sheet.
> 
> > > +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> > > +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> > > +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> > > +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> > > +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> > > +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> > > +
> > > +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> > > +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> > > +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> > > +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> > > +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> > > +
> > > +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> > > +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> > > +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> > > +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> > > +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> > > +
> > > +/* PHY clock control */
> > > +#define EXYNOS_4210_UPHYCLK			0x4
> > > +
> > > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> > > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> > > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> > > +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> > > +
> > > +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> > > +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> > > +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> > > +
> > > +/* PHY reset control */
> > > +#define EXYNOS_4210_UPHYRST			0x8
> > > +
> > > +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> > > +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> > > +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> > > +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> > > +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> > > +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> > > +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> > > +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> > > +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> > > +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)
> > > +
> > > +/* Isolation, configured in the power management unit */
> > > +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x704
> > > +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> > > +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x708
> > > +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> > > +
> > > +/* USBYPHY1 Floating prevention */
> > > +#define EXYNOS_4210_UPHY1CON			0x34
> > > +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> > > +
> > > +/* Mode switching SUB Device <-> Host */
> > > +#define EXYNOS_4210_MODE_SWITCH_OFFSET		0x21c
> > > +#define EXYNOS_4210_MODE_SWITCH_MASK		1
> > > +#define EXYNOS_4210_MODE_SWITCH_DEVICE		0
> > > +#define EXYNOS_4210_MODE_SWITCH_HOST		1
> > > +
> > > +enum exynos4210_phy_id {
> > > +	EXYNOS4210_DEVICE,
> > > +	EXYNOS4210_HOST,
> > > +	EXYNOS4210_HSIC0,
> > > +	EXYNOS4210_HSIC1,
> > > +	EXYNOS4210_NUM_PHYS,
> > > +};
> > > +
> > > +/*
> > > + * exynos4210_rate_to_clk() converts the supplied clock rate to
> the
> > > +value that
> > > + * can be written to the phy register.
> > > + */
> > > +static int exynos4210_rate_to_clk(unsigned long rate, u32 *reg) {
> > > +	switch (rate) {
> > > +	case 12 * MHZ:
> > > +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> > > +		break;
> > > +	case 24 * MHZ:
> > > +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> > > +		break;
> > > +	case 48 * MHZ:
> > > +		reg = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> >
> > %s/reg/*Reg
> 
> Thank you.
> 
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void exynos4210_isol(struct samsung_usb2_phy_instance *inst,
> > > +bool on) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	u32 offset;
> > > +	u32 mask;
> > > +
> > > +	switch (inst->cfg->id) {
> > > +	case EXYNOS4210_DEVICE:
> > > +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> > > +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> > > +		break;
> > > +	case EXYNOS4210_HOST:
> > > +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> > > +		mask = EXYNOS_4210_USB_ISOL_HOST;
> > > +		break;
> > > +	default:
> > > +		return;
> > > +	};
> > > +
> > > +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); }
> > > +
> > > +static void exynos4210_phy_pwr(struct samsung_usb2_phy_instance
> > > +*inst, bool on) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	u32 rstbits = 0;
> > > +	u32 phypwr = 0;
> > > +	u32 rst;
> > > +	u32 pwr;
> > > +
> > > +	switch (inst->cfg->id) {
> > > +	case EXYNOS4210_DEVICE:
> > > +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> > > +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> > > +		break;
> > > +	case EXYNOS4210_HOST:
> > > +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> > > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> > > +				EXYNOS_4210_URSTCON_PHY1_P0 |
> > > +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > > +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> > > +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> > > +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> > > +		break;
> > > +	case EXYNOS4210_HSIC0:
> > > +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> > > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > > +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> > > +		break;
> > > +	case EXYNOS4210_HSIC1:
> > > +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> > > +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> > > +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> > > +		break;
> > > +	};
> > > +
> > > +	if (on) {
> > > +		writel(inst->clk_reg_val, drv->reg_phy +
> > EXYNOS_4210_UPHYCLK);
> > > +
> > > +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > > +		pwr &= ~phypwr;
> > > +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > > +
> > > +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> > > +		rst |= rstbits;
> > > +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> > > +		udelay(10);
> > > +		rst &= ~rstbits;
> > > +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> >
> > Do you have to reset during every power on? IMO this reset should be
> > done once in phy_init.
> 
> It seems that this reset is neded by the hardware.
> 
> > > +	} else {
> > > +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > > +		pwr |= phypwr;
> > > +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> > > +	}
> > > +}
> > > +
> > > +static int exynos4210_power_on(struct samsung_usb2_phy_instance
> > > +*inst) {
> > > +	/* Order of initialisation is important - first power then
> > isolation */
> > > +	exynos4210_phy_pwr(inst, 1);
> > > +	exynos4210_isol(inst, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int exynos4210_power_off(struct samsung_usb2_phy_instance
> > > +*inst) {
> > > +	exynos4210_isol(inst, 1);
> > > +	exynos4210_phy_pwr(inst, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +static const struct samsung_usb2_common_phy exynos4210_phys[] = {
> > > +	{
> > > +		.label		= "device",
> > > +		.id		= EXYNOS4210_DEVICE,
> > > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > > +		.power_on	= exynos4210_power_on,
> > > +		.power_off	= exynos4210_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "host",
> > > +		.id		= EXYNOS4210_HOST,
> > > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > > +		.power_on	= exynos4210_power_on,
> > > +		.power_off	= exynos4210_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "hsic0",
> > > +		.id		= EXYNOS4210_HSIC0,
> > > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > > +		.power_on	= exynos4210_power_on,
> > > +		.power_off	= exynos4210_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "hsic1",
> > > +		.id		= EXYNOS4210_HSIC1,
> > > +		.rate_to_clk	= exynos4210_rate_to_clk,
> > > +		.power_on	= exynos4210_power_on,
> > > +		.power_off	= exynos4210_power_off,
> > > +	},
> > > +	{},
> > > +};
> > > +
> > > +const struct samsung_usb2_phy_config exynos4210_usb2_phy_config =
> {
> > > +	.num_phys		= EXYNOS4210_NUM_PHYS,
> > > +	.phys			= exynos4210_phys,
> > > +	.has_mode_switch	= 1,
> > > +};
> > > +
> > > diff --git a/drivers/phy/phy-exynos4212-usb2.c
> > > b/drivers/phy/phy-exynos4212-usb2.c
> > > new file mode 100644
> > > index 0000000..375ece0
> > > --- /dev/null
> > > +++ b/drivers/phy/phy-exynos4212-usb2.c
> > > @@ -0,0 +1,312 @@
> > > +/*
> > > + * Samsung S5P/EXYNOS SoC series USB PHY driver - Exynos 4212
> > support
> > > + *
> > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Kamil Debski <k.debski@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/regmap.h>
> > > +#include <linux/spinlock.h>
> >
> > You've included most of the above header files in phy-samsung-usb2.h
> > which you are including below.
> 
> Please see my reply above.
> 
> > > +#include "phy-samsung-usb2.h"
> > > +
> > > +/* Exynos USB PHY registers */
> > > +
> > > +/* PHY power control */
> > > +#define EXYNOS_4212_UPHYPWR			0x0
> > > +
> > > +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)
> >
> > use BIT() here and below..
> 
> All right.
> 
> > > +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> > > +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> > > +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> > > +#define EXYNOS_4212_UPHYPWR_DEV	( \
> > > +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> > > +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> > > +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> > > +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> > > +
> > > +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> > > +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> > > +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> > > +#define EXYNOS_4212_UPHYPWR_HOST ( \
> > > +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> > > +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> > > +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> > > +
> > > +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> > > +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> > > +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> > > +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> > > +
> > > +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> > > +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> > > +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> > > +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> > > +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> > > +
> > > +/* PHY clock control */
> > > +#define EXYNOS_4212_UPHYCLK			0x4
> > > +
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> > > +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> > > +
> > > +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> > > +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> > > +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> > > +
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> > > +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> > > +
> > > +/* PHY reset control */
> > > +#define EXYNOS_4212_UPHYRST			0x8
> > > +
> > > +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> > > +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> > > +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> > > +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> > > +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> > > +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> > > +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> > > +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> > > +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> > > +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> > > +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> > > +
> > > +/* Isolation, configured in the power management unit */
> > > +#define EXYNOS_4212_USB_ISOL_OFFSET		0x704
> > > +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> > > +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x708
> > > +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> > > +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x70c
> > > +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> > > +
> > > +/* Mode switching SUB Device <-> Host */
> > > +#define EXYNOS_4212_MODE_SWITCH_OFFSET		0x21c
> > > +#define EXYNOS_4212_MODE_SWITCH_MASK		1
> > > +#define EXYNOS_4212_MODE_SWITCH_DEVICE		0
> > > +#define EXYNOS_4212_MODE_SWITCH_HOST		1
> > > +
> > > +enum exynos4x12_phy_id {
> > > +	EXYNOS4212_DEVICE,
> > > +	EXYNOS4212_HOST,
> > > +	EXYNOS4212_HSIC0,
> > > +	EXYNOS4212_HSIC1,
> > > +	EXYNOS4212_NUM_PHYS,
> > > +};
> > > +
> > > +/*
> > > + * exynos4212_rate_to_clk() converts the supplied clock rate to
> the
> > > +value that
> > > + * can be written to the phy register.
> > > + */
> > > +static int exynos4212_rate_to_clk(unsigned long rate, u32 *reg) {
> > > +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> > > +
> > > +	switch (rate) {
> > > +	case 9600 * KHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> > > +		break;
> > > +	case 10 * MHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> > > +		break;
> > > +	case 12 * MHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> > > +		break;
> > > +	case 19200 * KHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> > > +		break;
> > > +	case 20 * MHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> > > +		break;
> > > +	case 24 * MHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> > > +		break;
> > > +	case 50 * MHZ:
> > > +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void exynos4212_isol(struct samsung_usb2_phy_instance *inst,
> > > +bool on) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	u32 offset;
> > > +	u32 mask;
> > > +
> > > +	switch (inst->cfg->id) {
> > > +	case EXYNOS4212_DEVICE:
> > > +	case EXYNOS4212_HOST:
> > > +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> > > +		mask = EXYNOS_4212_USB_ISOL_OTG;
> > > +		break;
> > > +	case EXYNOS4212_HSIC0:
> > > +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> > > +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> > > +		break;
> > > +	case EXYNOS4212_HSIC1:
> > > +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> > > +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> > > +		break;
> > > +	default:
> > > +		return;
> > > +	};
> > > +
> > > +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); }
> > > +
> > > +static void exynos4212_phy_pwr(struct samsung_usb2_phy_instance
> > > +*inst, bool on) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	u32 rstbits = 0;
> > > +	u32 phypwr = 0;
> > > +	u32 rst;
> > > +	u32 pwr;
> > > +
> > > +	switch (inst->cfg->id) {
> > > +	case EXYNOS4212_DEVICE:
> > > +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> > > +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> > > +		break;
> > > +	case EXYNOS4212_HOST:
> > > +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> > > +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> > > +		break;
> > > +	case EXYNOS4212_HSIC0:
> > > +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> > > +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> > > +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> > > +				EXYNOS_4212_URSTCON_HOST_PHY;
> > > +		break;
> > > +	case EXYNOS4212_HSIC1:
> > > +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> > > +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> > > +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> > > +		break;
> > > +	};
> > > +
> > > +	if (on) {
> > > +		writel(inst->clk_reg_val, drv->reg_phy +
> > EXYNOS_4212_UPHYCLK);
> > > +
> > > +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > > +		pwr &= ~phypwr;
> > > +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > > +
> > > +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> > > +		rst |= rstbits;
> > > +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> > > +		udelay(10);
> > > +		rst &= ~rstbits;
> > > +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> >
> > reset should be done once in init?
> 
> The hardware seems to need this reset.
> 
> > > +	} else {
> > > +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > > +		pwr |= phypwr;
> > > +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> > > +	}
> > > +}
> > > +
> > > +static int exynos4212_power_on(struct samsung_usb2_phy_instance
> > > +*inst) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +
> > > +	inst->enabled = 1;
> > > +	exynos4212_phy_pwr(inst, 1);
> > > +	exynos4212_isol(inst, 0);
> > > +
> > > +	/* Power on the device, as it is necessary for HSIC to work */
> >
> > This looks weird. How powering on the 'device PHY' makes 'HSIC PHY'
> to
> > work?
> 
> I agree with you that it looks weird, but I found this necessary to be
> done.
> 
> > > +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> > > +		struct samsung_usb2_phy_instance *device =
> > > +					&drv->instances[EXYNOS4212_DEVICE];
> > > +		exynos4212_phy_pwr(device, 1);
> > > +		exynos4212_isol(device, 0);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int exynos4212_power_off(struct samsung_usb2_phy_instance
> > > +*inst) {
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	struct samsung_usb2_phy_instance *device =
> > > +&drv->instances[EXYNOS4212_DEVICE];
> > > +
> > > +	inst->enabled = 0;
> > > +	exynos4212_isol(inst, 1);
> > > +	exynos4212_phy_pwr(inst, 0);
> > > +
> > > +	if (inst->cfg->id == EXYNOS4212_HSIC0 && !device->enabled) {
> > > +		exynos4212_isol(device, 1);
> > > +		exynos4212_phy_pwr(device, 0);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +static const struct samsung_usb2_common_phy exynos4212_phys[] = {
> > > +	{
> > > +		.label		= "device",
> > > +		.id		= EXYNOS4212_DEVICE,
> > > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > > +		.power_on	= exynos4212_power_on,
> > > +		.power_off	= exynos4212_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "host",
> > > +		.id		= EXYNOS4212_HOST,
> > > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > > +		.power_on	= exynos4212_power_on,
> > > +		.power_off	= exynos4212_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "hsic0",
> > > +		.id		= EXYNOS4212_HSIC0,
> > > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > > +		.power_on	= exynos4212_power_on,
> > > +		.power_off	= exynos4212_power_off,
> > > +	},
> > > +	{
> > > +		.label		= "hsic1",
> > > +		.id		= EXYNOS4212_HSIC1,
> > > +		.rate_to_clk	= exynos4212_rate_to_clk,
> > > +		.power_on	= exynos4212_power_on,
> > > +		.power_off	= exynos4212_power_off,
> > > +	},
> > > +	{},
> > > +};
> > > +
> > > +const struct samsung_usb2_phy_config exynos4212_usb2_phy_config =
> {
> > > +	.num_phys		= EXYNOS4212_NUM_PHYS,
> > > +	.phys			= exynos4212_phys,
> > > +	.has_mode_switch	= 1,
> > > +};
> > > +
> > > diff --git a/drivers/phy/phy-samsung-usb2.c
> > > b/drivers/phy/phy-samsung-usb2.c new file mode 100644 index
> > > 0000000..804ec77
> > > --- /dev/null
> > > +++ b/drivers/phy/phy-samsung-usb2.c
> > > @@ -0,0 +1,228 @@
> > > +/*
> > > + * Samsung SoC USB 1.1/2.0 PHY driver
> > > + *
> > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Kamil Debski <k.debski@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/mfd/syscon.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/spinlock.h>
> >
> > You've included most of the above header files in phy-samsung-usb2.h
> > which you are including below.
> 
> Please see my answer above.
> 
> > > +#include "phy-samsung-usb2.h"
> > > +
> > > +static int samsung_usb2_phy_power_on(struct phy *phy) {
> > > +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	int ret;
> > > +
> > > +	dev_dbg(drv->dev, "Request to power_on \"%s\" usb phy\n",
> > > +							inst->cfg->label);
> > > +	ret = clk_prepare_enable(drv->clk);
> > > +	if (ret)
> > > +		goto err_main_clk;
> > > +	ret = clk_prepare_enable(inst->clk);
> > > +	if (ret)
> > > +		goto err_instance_clk;
> > > +	inst->rate = clk_get_rate(inst->clk);
> > > +	if (inst->cfg->rate_to_clk) {
> > > +		ret = inst->cfg->rate_to_clk(inst->rate, &inst-
> > >clk_reg_val);
> > > +		if (ret)
> > > +			goto err_get_rate;
> > > +	}
> > > +	if (inst->cfg->power_on) {
> > > +		spin_lock(&drv->lock);
> > > +		ret = inst->cfg->power_on(inst);
> > > +		spin_unlock(&drv->lock);
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_get_rate:
> > > +	clk_disable_unprepare(inst->clk);
> > > +err_instance_clk:
> > > +	clk_disable_unprepare(drv->clk);
> > > +err_main_clk:
> > > +	return ret;
> > > +}
> > > +
> > > +static int samsung_usb2_phy_power_off(struct phy *phy) {
> > > +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> > > +	struct samsung_usb2_phy_driver *drv = inst->drv;
> > > +	int ret = 0;
> > > +
> > > +	dev_dbg(drv->dev, "Request to power_off \"%s\" usb phy\n",
> > > +							inst->cfg->label);
> > > +	if (inst->cfg->power_off) {
> > > +		spin_lock(&drv->lock);
> > > +		ret = inst->cfg->power_off(inst);
> > > +		spin_unlock(&drv->lock);
> > > +	}
> > > +	clk_disable_unprepare(inst->clk);
> > > +	clk_disable_unprepare(drv->clk);
> > > +	return ret;
> > > +}
> > > +
> > > +static struct phy_ops samsung_usb2_phy_ops = {
> > > +	.power_on	= samsung_usb2_phy_power_on,
> > > +	.power_off	= samsung_usb2_phy_power_off,
> > > +	.owner		= THIS_MODULE,
> > > +};
> > > +
> > > +static struct phy *samsung_usb2_phy_xlate(struct device *dev,
> > > +					struct of_phandle_args *args)
> > > +{
> > > +	struct samsung_usb2_phy_driver *drv;
> > > +
> > > +	drv = dev_get_drvdata(dev);
> > > +	if (!drv)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> > > +		return ERR_PTR(-ENODEV);
> > > +
> > > +	return drv->instances[args->args[0]].phy;
> > > +}
> > > +
> > > +static const struct of_device_id samsung_usb2_phy_of_match[] = {
> > > +#ifdef CONFIG_PHY_EXYNOS4210_USB2
> > > +	{
> > > +		.compatible = "samsung,exynos4210-usb2-phy",
> > > +		.data = &exynos4210_usb2_phy_config,
> > > +	},
> > > +#endif
> > > +#ifdef CONFIG_PHY_EXYNOS4212_USB2
> > > +	{
> > > +		.compatible = "samsung,exynos4212-usb2-phy",
> > > +		.data = &exynos4212_usb2_phy_config,
> > > +	},
> > > +#endif
> > > +	{ },
> > > +};
> >
> > I think we've had enough discussion about this approach. Let's get
> the
> > opinion of others too. Felipe? Greg?
> 
> Good idea.
> 
> > Summary:
> > We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
> > almost similar register map [1] and a samsung helper driver for these
> > two drivers.
> 
> I would not call them separate drivers. It's a single USB 2.0 driver
> with the option to include support for various SoCs. This patchset adds:
> Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another
> person is working on supporting S3C6410.
> 
> > These two PHY drivers populate the function pointers in the helper
> > driver. So any phy_ops will first invoke the helper driver which will
> > then invoke the corresponding phy driver.
> >
> > [1] -> http://www.diffchecker.com/7yno1uvk
> 
> Come on, this diff only includes the registers part of the file.
> The following functions are also different:
> - exynos421*_rate_to_clk
> - exynos421*_isol
> - exynos421*_phy_pwr
> - exynos421*_power_on
> - exynos421*_power_on
> 
> It seems that the file is too large for the tool. But still this makes
> a false impression that only registers are different.
> 
> > Advantages:
> > * (more) clean and readable
> > * helper driver can be used with other PHY drivers which will be
> added
> > soon
> >
> > Disadvantages:
> > * code duplication
> 
> I would say that actually in this case less code is duplicated. Having
> Separate drivers would mean that most of the phy-samsung-usb2.c file
> has to be repeated. That is 240 times 4 (and surely more in the future,
> as this patchset adds support for 4 SoCs). Which is almost 1000 lines
> more.
> 
> >
> > Maybe having a helper driver makes sense when we have other samsung
> > PHY drivers added but not sure if it's needed here for
> EXYNOS4210_USB2
> > and
> > EXYNOS4212_USB2
> >
> > Need your inputs on what you think about this.
> 
> Yes, I would also welcome other people's opinions.
> 
> > > +
> > > +static int samsung_usb2_phy_probe(struct platform_device *pdev) {
> > > +	const struct of_device_id *match;
> > > +	const struct samsung_usb2_phy_config *cfg;
> > > +	struct clk *clk;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct phy_provider *phy_provider;
> > > +	struct resource *mem;
> > > +	struct samsung_usb2_phy_driver *drv;
> > > +	int i;
> > > +
> > > +	if (!pdev->dev.of_node) {
> > > +		dev_err(dev, "This driver is required to be instantiated
> > from device tree\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	match = of_match_node(samsung_usb2_phy_of_match, pdev-
> > >dev.of_node);
> > > +	if (!match) {
> > > +		dev_err(dev, "of_match_node() failed\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	cfg = match->data;
> > > +
> > > +	drv = devm_kzalloc(dev, sizeof(struct samsung_usb2_phy_driver) +
> > > +		cfg->num_phys * sizeof(struct samsung_usb2_phy_instance),
> > GFP_KERNEL);
> > > +	if (!drv)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(dev, drv);
> > > +	spin_lock_init(&drv->lock);
> > > +
> > > +	drv->cfg = cfg;
> > > +	drv->dev = dev;
> > > +
> > > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> > > +	if (IS_ERR(drv->reg_phy)) {
> > > +		dev_err(dev, "Failed to map register memory (phy)\n");
> > > +		return PTR_ERR(drv->reg_phy);
> > > +	}
> > > +
> > > +	drv->reg_pmu = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > > +		"samsung,pmureg-phandle");
> > > +	if (IS_ERR(drv->reg_pmu)) {
> > > +		dev_err(dev, "Failed to map PMU registers (via syscon)\n");
> > > +		return PTR_ERR(drv->reg_pmu);
> > > +	}
> > > +
> > > +	if (drv->cfg->has_mode_switch) {
> > > +		drv->reg_sys = syscon_regmap_lookup_by_phandle(
> > > +				pdev->dev.of_node,
"samsung,sysreg-phandle");
> > > +		if (IS_ERR(drv->reg_sys)) {
> > > +			dev_err(dev, "Failed to map system registers (via
> > syscon)\n");
> > > +			return PTR_ERR(drv->reg_sys);
> > > +		}
> > > +	}
> > > +
> > > +	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);
> > > +	}
> > > +
> > > +	for (i = 0; i < drv->cfg->num_phys; i++) {
> > > +		char *label = drv->cfg->phys[i].label;
> > > +		struct samsung_usb2_phy_instance *p = &drv->instances[i];
> > > +
> > > +		dev_dbg(dev, "Creating phy \"%s\"\n", label);
> > > +		p->phy = devm_phy_create(dev, &samsung_usb2_phy_ops, NULL);
> > > +		if (IS_ERR(p->phy)) {
> > > +			dev_err(drv->dev, "Failed to create usb2_phy
> > \"%s\"\n",
> > > +						label);
> > > +			return PTR_ERR(p->phy);
> > > +		}
> > > +
> > > +		p->cfg = &drv->cfg->phys[i];
> > > +		p->drv = drv;
> > > +		phy_set_drvdata(p->phy, p);
> > > +
> > > +		clk = devm_clk_get(dev, p->cfg->label);
> > > +		if (IS_ERR(clk)) {
> > > +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> > > +
p->cfg->label);
> > > +			return PTR_ERR(clk);
> > > +		}
> > > +		p->clk = clk;
> > > +	}
> > > +
> > > +	phy_provider = devm_of_phy_provider_register(dev,
> > > +
samsung_usb2_phy_xlate);
> > > +	if (IS_ERR(phy_provider)) {
> > > +		dev_err(drv->dev, "Failed to register phy provider\n");
> > > +		return PTR_ERR(phy_provider);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver samsung_usb2_phy_driver = {
> > > +	.probe	= samsung_usb2_phy_probe,
> > > +	.driver = {
> > > +		.of_match_table	= samsung_usb2_phy_of_match,
> > > +		.name		= "samsung-usb2-phy",
> > > +		.owner		= THIS_MODULE,
> > > +	}
> > > +};
> > > +
> > > +module_platform_driver(samsung_usb2_phy_driver);
> > > +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> > > +MODULE_AUTHOR("Kamil Debski <k.debski@samsung.com>");
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_ALIAS("platform:samsung-usb2-phy");
> > > +
> > > diff --git a/drivers/phy/phy-samsung-usb2.h
> > > b/drivers/phy/phy-samsung-usb2.h new file mode 100644 index
> > > 0000000..cd12477
> > > --- /dev/null
> > > +++ b/drivers/phy/phy-samsung-usb2.h
> > > @@ -0,0 +1,72 @@
> > > +/*
> > > + * Samsung SoC USB 1.1/2.0 PHY driver
> > > + *
> > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Kamil Debski <k.debski@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.
> > > + */
> > > +
> > > +#ifndef _PHY_EXYNOS_USB2_H
> > > +#define _PHY_EXYNOS_USB2_H
> > > +
> > > +#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/spinlock.h>
> > > +
> > > +#define KHZ 1000
> > > +#define MHZ (KHZ * KHZ)
> > > +
> > > +struct samsung_usb2_phy_driver;
> > > +struct samsung_usb2_phy_instance;
> > > +struct samsung_usb2_phy_config;
> > > +
> > > +struct samsung_usb2_phy_instance {
> > > +	struct samsung_usb2_phy_driver *drv;
> > > +	struct phy *phy;
> > > +	const struct samsung_usb2_common_phy *cfg;
> > > +	char enabled;
> > > +	struct clk *clk;
> > > +	u32 clk_reg_val;
> > > +	unsigned long rate;
> > > +};
> > > +
> > > +struct samsung_usb2_phy_driver {
> > > +	struct device *dev;
> > > +	spinlock_t lock;
> > > +	void __iomem *reg_phy;
> > > +	struct regmap *reg_sys;
> > > +	struct regmap *reg_pmu;
> > > +	const struct samsung_usb2_phy_config *cfg;
> > > +	struct clk *clk;
> > > +	struct samsung_usb2_phy_instance instances[0]; };
> > > +
> > > +struct samsung_usb2_common_phy {
> > > +	char *label;
> > > +	unsigned int id;
> > > +	int (*rate_to_clk)(unsigned long, u32 *);
> > > +	int (*power_on)(struct samsung_usb2_phy_instance *);
> > > +	int (*power_off)(struct samsung_usb2_phy_instance *); };
> > > +
> > > +
> > > +struct samsung_usb2_phy_config {
> > > +	int num_phys;
> > > +	const struct samsung_usb2_common_phy *phys;
> > > +	char has_mode_switch;
> >
> > u8 instead?
> 
> Do we really need to specify that we need 8bits? Why do you think char
> is wrong?
> 
> Please read this paragraph from LDD3:
> "Sometimes kernel code requires data items of a specific size, perhaps
> to match predefined binary structures,* to communicate with user space,
> or to align data within structures by inserting "padding" fields (but
> refer to the section "Data Alignment" for information about alignment
> issues)."
> Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf
> 
> has_mode_switch is only a flag 0 or 1. Never written anywhere in
> hardware registers. Used in an if somewhere in code. Give me a good
> reason why do you think it should be explicitly 8 bit long.
> 

On the second thought, we both are wrong as bool would be the best choice
here. Have a good weekend.

Best wishes,

Hi,

On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:
> Hi Kishon,
>
> Thank you for the review.
>
>> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
>> Sent: Friday, December 06, 2013 11:59 AM
>>
>> Hi,
>>
>> On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
>>> Add a new driver for the Exynos USB PHY. The new driver uses the
>>> generic PHY framework. The driver includes support for the Exynos
>> 4x10
>>> and 4x12 SoC families.
>>>
>>> Signed-off-by: Kamil Debski <k.debski@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---


<snip>
.
.
>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
>>> d0caae9..9f4befd 100644
>>> --- a/drivers/phy/Makefile
>>> +++ b/drivers/phy/Makefile
>>> @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
>>> +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
>>> +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
>>> diff --git a/drivers/phy/phy-exynos4210-usb2.c
>>> b/drivers/phy/phy-exynos4210-usb2.c
>>> new file mode 100644
>>> index 0000000..a02e5c2
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-exynos4210-usb2.c
>>> @@ -0,0 +1,264 @@
>>> +/*
>>> + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
>>> + *
>>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>>> + * Author: Kamil Debski <k.debski@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/regmap.h>
>>> +#include <linux/spinlock.h>
>>
>> You've included most of the above header files in phy-samsung-usb2.h
>> which you are including below.
>
> I agree that includes in phy-samsung-usb2.h could use a cleanup. On the
> other
> hand my opinion is that a .c file should include all .h files that are used
> in
> this .c file. Relaying on .h file to include another .h doesn't seem good to
> me.

then remove it in .h file.
>
>>> +#include "phy-samsung-usb2.h"
>>> +
>>> +/* Exynos USB PHY registers */
>>> +
>>> +/* PHY power control */
>>> +#define EXYNOS_4210_UPHYPWR			0x0
>>> +
>>> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
>>
>> use BIT() here and everywhere below.
>

<snip>
.
.

>>> +#ifdef CONFIG_PHY_EXYNOS4212_USB2
>>> +	{
>>> +		.compatible = "samsung,exynos4212-usb2-phy",
>>> +		.data = &exynos4212_usb2_phy_config,
>>> +	},
>>> +#endif
>>> +	{ },
>>> +};
>>
>> I think we've had enough discussion about this approach. Let's get the
>> opinion of others too. Felipe? Greg?
>
> Good idea.
>
>> Summary:
>> We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
>> almost similar register map [1] and a samsung helper driver for these
>> two drivers.
>
> I would not call them separate drivers. It's a single USB 2.0 driver with
> the option to include support for various SoCs. This patchset adds:
> Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another
> person is working on supporting S3C6410.
>
>> These two PHY drivers populate the function pointers in the helper
>> driver. So any phy_ops will first invoke the helper driver which will
>> then invoke the corresponding phy driver.
>>
>> [1] -> http://www.diffchecker.com/7yno1uvk
>
> Come on, this diff only includes the registers part of the file.
> The following functions are also different:
> - exynos421*_rate_to_clk
> - exynos421*_isol
> - exynos421*_phy_pwr
> - exynos421*_power_on
> - exynos421*_power_on

But most of the differences is because your 4212 has additional features 
in HSIC and supports more clock rates.
>
> It seems that the file is too large for the tool. But still this makes a
> false impression that only registers are different.
>
>> Advantages:
>> * (more) clean and readable
>> * helper driver can be used with other PHY drivers which will be added
>> soon
>>
>> Disadvantages:
>> * code duplication
>
> I would say that actually in this case less code is duplicated. Having
> Separate drivers would mean that most of the phy-samsung-usb2.c file has

I actually meant a single driver for 4210 and 4212.

your current code has separate drivers for different versions of the 
same IP. If you have a single driver for the different versions, it will 
lead to a lot less code duplication (hint: I've given the exact 'same' 
comment at-least twice in this patch). There are quite a few examples in 
the kernel where the same driver is used for multiple versions of the IP.
> to be repeated. That is 240 times 4 (and surely more in the future, as
> this patchset adds support for 4 SoCs). Which is almost 1000 lines more.
>
>>
>> Maybe having a helper driver makes sense when we have other samsung PHY
>> drivers added but not sure if it's needed here for EXYNOS4210_USB2 and
>> EXYNOS4212_USB2
>>
>> Need your inputs on what you think about this.
>
> Yes, I would also welcome other people's opinions.
>
>>> +
>>> +static int samsung_usb2_phy_probe(struct platform_device *pdev) {
>>> +	const struct of_device_id *match;
>>> +	const struct samsung_usb2_phy_config *cfg;
>>> +	struct clk *clk;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct phy_provider *phy_provider;
>>> +	struct resource *mem;
>>> +	struct samsung_usb2_phy_driver *drv;
>>> +	int i;
>>> +
>>> +	if (!pdev->dev.of_node) {
>>> +		dev_err(dev, "This driver is required to be instantiated
>> from device tree\n");
>>> +		return -EINVAL;
>>> +	}
>>> +

<snip>
.
.
>>> +	int (*power_on)(struct samsung_usb2_phy_instance *);
>>> +	int (*power_off)(struct samsung_usb2_phy_instance *); };
>>> +
>>> +
>>> +struct samsung_usb2_phy_config {
>>> +	int num_phys;
>>> +	const struct samsung_usb2_common_phy *phys;
>>> +	char has_mode_switch;
>>
>> u8 instead?
>
> Do we really need to specify that we need 8bits? Why do you think
> char is wrong?

Do you really assign a char? Having a char data type and assigning an 
integer is misleading.
>
> Please read this paragraph from LDD3:
> "Sometimes kernel code requires data items of a specific size,
> perhaps to match predefined binary structures,* to communicate with
> user space, or to align data within structures by inserting
> "padding" fields (but refer to the section "Data Alignment" for
> information about alignment issues)."
> Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf
>
> has_mode_switch is only a flag 0 or 1. Never written anywhere in
> hardware registers. Used in an if somewhere in code. Give me a good
> reason why do you think it should be explicitly 8 bit long.

I just thought you created a char type to assign an integer value is you 
wanted to some data type which is 8 bits long. If it is for any other 
reason you used a char data type, pls let us know.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi, 

> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> Sent: Monday, December 09, 2013 8:56 AM
> 
> Hi,
> 
> On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:
> > Hi Kishon,
> >
> > Thank you for the review.
> >
> >> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> >> Sent: Friday, December 06, 2013 11:59 AM
> >>
> >> Hi,
> >>
> >> On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
> >>> Add a new driver for the Exynos USB PHY. The new driver uses the
> >>> generic PHY framework. The driver includes support for the Exynos
> >> 4x10
> >>> and 4x12 SoC families.
> >>>
> >>> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>> ---
> 
> 
> <snip>
> .
> .
> >>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
> >>> d0caae9..9f4befd 100644
> >>> --- a/drivers/phy/Makefile
> >>> +++ b/drivers/phy/Makefile
> >>> @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
> >>> +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
> >>> +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
> >>> diff --git a/drivers/phy/phy-exynos4210-usb2.c
> >>> b/drivers/phy/phy-exynos4210-usb2.c
> >>> new file mode 100644
> >>> index 0000000..a02e5c2
> >>> --- /dev/null
> >>> +++ b/drivers/phy/phy-exynos4210-usb2.c
> >>> @@ -0,0 +1,264 @@
> >>> +/*
> >>> + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
> >>> + *
> >>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> >>> + * Author: Kamil Debski <k.debski@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/regmap.h>
> >>> +#include <linux/spinlock.h>
> >>
> >> You've included most of the above header files in phy-samsung-usb2.h
> >> which you are including below.
> >
> > I agree that includes in phy-samsung-usb2.h could use a cleanup. On
> > the other hand my opinion is that a .c file should include all .h
> > files that are used in this .c file. Relaying on .h file to include
> > another .h doesn't seem good to me.
> 
> then remove it in .h file.
> >
> >>> +#include "phy-samsung-usb2.h"
> >>> +
> >>> +/* Exynos USB PHY registers */
> >>> +
> >>> +/* PHY power control */
> >>> +#define EXYNOS_4210_UPHYPWR			0x0
> >>> +
> >>> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> >>
> >> use BIT() here and everywhere below.
> >
> 
> <snip>
> .
> .
> 
> >>> +#ifdef CONFIG_PHY_EXYNOS4212_USB2
> >>> +	{
> >>> +		.compatible = "samsung,exynos4212-usb2-phy",
> >>> +		.data = &exynos4212_usb2_phy_config,
> >>> +	},
> >>> +#endif
> >>> +	{ },
> >>> +};
> >>
> >> I think we've had enough discussion about this approach. Let's get
> >> the opinion of others too. Felipe? Greg?
> >
> > Good idea.
> >
> >> Summary:
> >> We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
> >> almost similar register map [1] and a samsung helper driver for
> these
> >> two drivers.
> >
> > I would not call them separate drivers. It's a single USB 2.0 driver
> > with the option to include support for various SoCs. This patchset
> adds:
> > Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that
> > another person is working on supporting S3C6410.
> >
> >> These two PHY drivers populate the function pointers in the helper
> >> driver. So any phy_ops will first invoke the helper driver which
> will
> >> then invoke the corresponding phy driver.
> >>
> >> [1] -> http://www.diffchecker.com/7yno1uvk
> >
> > Come on, this diff only includes the registers part of the file.
> > The following functions are also different:
> > - exynos421*_rate_to_clk
> > - exynos421*_isol
> > - exynos421*_phy_pwr
> > - exynos421*_power_on
> > - exynos421*_power_on
> 
> But most of the differences is because your 4212 has additional
> features in HSIC and supports more clock rates.
> >
> > It seems that the file is too large for the tool. But still this
> makes
> > a false impression that only registers are different.
> >
> >> Advantages:
> >> * (more) clean and readable
> >> * helper driver can be used with other PHY drivers which will be
> >> added soon
> >>
> >> Disadvantages:
> >> * code duplication
> >
> > I would say that actually in this case less code is duplicated.
> Having
> > Separate drivers would mean that most of the phy-samsung-usb2.c file
> > has
> 
> I actually meant a single driver for 4210 and 4212.
> 
> your current code has separate drivers for different versions of the
> same IP. If you have a single driver for the different versions, it
> will lead to a lot less code duplication (hint: I'v given the exact
> 'same'
> comment at-least twice in this patch).

You wrote more than twice, I know. I also replied quite a few times. 
I want to explain that this is not a driver for Exynos 4210 and 4212 only.
It is a driver for more SoCs with different IPs.
Like Exynos 5250, S5PV210 and I am sure many more are to follow.
The other two patches in this series add new functionality to this driver
and are not separate drivers.

It is a single driver with custom functions for separate SoC and separate
versions of the same IP. In my opinion it will lead to less code in the
kernel. But we have different opinion on that :) We agree that the code
is more clear and it is easier to manage and add support for new SoC.
Maybe we should focus more on having a clear and readable code?

Kishon, if we were talking about Exynos 4210 and 4212 only, then I could
agree with you. But this driver covers more SoCs.

> There are quite a few examples
> in the kernel where the same driver is used for multiple versions of
> the IP.
> > to be repeated. That is 240 times 4 (and surely more in the future,
> as
> > this patchset adds support for 4 SoCs). Which is almost 1000 lines
> more.
> >
> >>
> >> Maybe having a helper driver makes sense when we have other samsung
> >> PHY drivers added but not sure if it's needed here for
> >> EXYNOS4210_USB2 and
> >> EXYNOS4212_USB2
> >>
> >> Need your inputs on what you think about this.
> >
> > Yes, I would also welcome other people's opinions.
> >
> >>> +
> >>> +static int samsung_usb2_phy_probe(struct platform_device *pdev) {
> >>> +	const struct of_device_id *match;
> >>> +	const struct samsung_usb2_phy_config *cfg;
> >>> +	struct clk *clk;
> >>> +	struct device *dev = &pdev->dev;
> >>> +	struct phy_provider *phy_provider;
> >>> +	struct resource *mem;
> >>> +	struct samsung_usb2_phy_driver *drv;
> >>> +	int i;
> >>> +
> >>> +	if (!pdev->dev.of_node) {
> >>> +		dev_err(dev, "This driver is required to be instantiated
> >> from device tree\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> 
> <snip>
> .
> .
> >>> +	int (*power_on)(struct samsung_usb2_phy_instance *);
> >>> +	int (*power_off)(struct samsung_usb2_phy_instance *); };
> >>> +
> >>> +
> >>> +struct samsung_usb2_phy_config {
> >>> +	int num_phys;
> >>> +	const struct samsung_usb2_common_phy *phys;
> >>> +	char has_mode_switch;
> >>
> >> u8 instead?
> >
> > Do we really need to specify that we need 8bits? Why do you think
> char
> > is wrong?
> 
> Do you really assign a char? Having a char data type and assigning an
> integer is misleading.
> >
> > Please read this paragraph from LDD3:
> > "Sometimes kernel code requires data items of a specific size,
> perhaps
> > to match predefined binary structures,* to communicate with user
> > space, or to align data within structures by inserting "padding"
> > fields (but refer to the section "Data Alignment" for information
> > about alignment issues)."
> > Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf
> >
> > has_mode_switch is only a flag 0 or 1. Never written anywhere in
> > hardware registers. Used in an if somewhere in code. Give me a good
> > reason why do you think it should be explicitly 8 bit long.
> 
> I just thought you created a char type to assign an integer value is
> you wanted to some data type which is 8 bits long. If it is for any
> other reason you used a char data type, pls let us know.
> 

You must have missed the message I sent soon after the one you replied
to - bool is the best candidate for has_mode_switch.

Best wishes,

Hi Kamil,

Same USB2.0 PHY may be used by several HCDs, for example EHCI and OHCI.
Consider the situation, when EHCI stops using the PHY and calls power_off,
then OHCI becomes non-operational. In other words, PHY power_on and
power_off calls must be balanced. 

Shall we handle it in your driver? (usage count?)

> Add a new driver for the Exynos USB PHY. The new driver uses the
> generic
> PHY framework. The driver includes support for the Exynos 4x10 and 4x12
> SoC families.
> 
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/phy/samsung-usbphy.txt     |   54 ++++
>  drivers/phy/Kconfig                                |   20 ++
>  drivers/phy/Makefile                               |    3 +
>  drivers/phy/phy-exynos4210-usb2.c                  |  264
> +++++++++++++++++
>  drivers/phy/phy-exynos4212-usb2.c                  |  312
> ++++++++++++++++++++
>  drivers/phy/phy-samsung-usb2.c                     |  228
> ++++++++++++++
>  drivers/phy/phy-samsung-usb2.h                     |   72 +++++
>  7 files changed, 953 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/samsung-
> usbphy.txt
>  create mode 100644 drivers/phy/phy-exynos4210-usb2.c
>  create mode 100644 drivers/phy/phy-exynos4212-usb2.c
>  create mode 100644 drivers/phy/phy-samsung-usb2.c
>  create mode 100644 drivers/phy/phy-samsung-usb2.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> new file mode 100644
> index 0000000..cadbf70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> @@ -0,0 +1,54 @@
> +Samsung S5P/EXYNOS SoC series USB PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-usb2-phy"
> +	- "samsung,exynos4212-usb2-phy"
> +- reg : a list of registers used by phy driver
> +	- first and obligatory is the location of phy modules registers
> +- samsung,sysreg-phandle - handle to syscon used to control the system
> registers
> +- samsung,pmureg-phandle - handle to syscon used to control PMU
> registers
> +- #phy-cells : from the generic phy bindings, must be 1;
> +- clocks and clock-names:
> +	- the "phy" clocks is required by the phy module
> +	- next for each of the phys a clock has to be assidned, this
> clock
> +	  will be used to determine clocking frequency for the phys
> +	  (the labels are specified in the paragraph below)
> +
> +The first phandle argument in the PHY specifier identifies the PHY,
> its
> +meaning is compatible dependent. For the currently supported SoCs
> (Exynos 4210
> +and Exynos 4212) it is as follows:
> +  0 - USB device ("device"),
> +  1 - USB host ("host"),
> +  2 - HSIC0 ("hsic0"),
> +  3 - HSIC1 ("hsic1"),
> +
> +Exynos 4210 and Exynos 4212 use mode switching and require that mode
> switch
> +register is supplied.
> +
> +Example:
> +
> +For Exynos 4412 (compatible with Exynos 4212):
> +
> +usbphy: phy@125B0000 {
> +	compatible = "samsung,exynos4212-usb2-phy";
> +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> +							<&clock 2>;
> +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> +	status = "okay";
> +	#phy-cells = <1>;
> +	samsung,sysreg-phandle = <&sys_reg>;
> +	samsung,pmureg-phandle = <&pmu_reg>;
> +};
> +
> +Then the PHY can be used in other nodes such as:
> +
> +phy-consumer@12340000 {
> +	phys = <&usbphy 2>;
> +	phy-names = "phy";
> +};
> +
> +Refer to DT bindings documentation of particular PHY consumer devices
> for more
> +information about required PHYs and the way of specification.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index a344f3d..b29018f 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
>  	help
>  	  Support for Display Port PHY found on Samsung EXYNOS SoCs.
> 
> +config PHY_SAMSUNG_USB2
> +	tristate "Samsung USB 2.0 PHY driver"
> +	help
> +	  Enable this to support Samsung USB phy helper driver for
> Samsung SoCs.
> +	  This driver provides common interface to interact, for Samsung
> +	  USB 2.0 PHY driver.
> +
> +config PHY_EXYNOS4210_USB2
> +	bool "Support for Exynos 4210"
> +	depends on PHY_SAMSUNG_USB2
> +	depends on CPU_EXYNOS4210
> +	help
> +	  Enable USB PHY support for Exynos 4210
> +
> +config PHY_EXYNOS4212_USB2
> +	bool "Support for Exynos 4212"
> +	depends on PHY_SAMSUNG_USB2
> +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> +	help
> +	  Enable USB PHY support for Exynos 4212
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index d0caae9..9f4befd 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -7,3 +7,6 @@ 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_SAMSUNG_USB2)		+= phy-samsung-usb2.o
> +obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
> +obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o
> diff --git a/drivers/phy/phy-exynos4210-usb2.c b/drivers/phy/phy-
> exynos4210-usb2.c
> new file mode 100644
> index 0000000..a02e5c2
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4210-usb2.c
> @@ -0,0 +1,264 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/regmap.h>
> +#include <linux/spinlock.h>
> +#include "phy-samsung-usb2.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4210_UPHYPWR			0x0
> +
> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)
> +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)
> +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4210_UPHYCLK			0x4
> +
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +
> +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +/* PHY reset control */
> +#define EXYNOS_4210_UPHYRST			0x8
> +
> +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x704
> +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x708
> +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> +
> +/* USBYPHY1 Floating prevention */
> +#define EXYNOS_4210_UPHY1CON			0x34
> +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> +
> +/* Mode switching SUB Device <-> Host */
> +#define EXYNOS_4210_MODE_SWITCH_OFFSET		0x21c
> +#define EXYNOS_4210_MODE_SWITCH_MASK		1
> +#define EXYNOS_4210_MODE_SWITCH_DEVICE		0
> +#define EXYNOS_4210_MODE_SWITCH_HOST		1
> +
> +enum exynos4210_phy_id {
> +	EXYNOS4210_DEVICE,
> +	EXYNOS4210_HOST,
> +	EXYNOS4210_HSIC0,
> +	EXYNOS4210_HSIC1,
> +	EXYNOS4210_NUM_PHYS,
> +};
> +
> +/*
> + * exynos4210_rate_to_clk() converts the supplied clock rate to the
> value that
> + * can be written to the phy register.
> + */
> +static int exynos4210_rate_to_clk(unsigned long rate, u32 *reg)
> +{
> +	switch (rate) {
> +	case 12 * MHZ:
> +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 24 * MHZ:
> +		*reg = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 48 * MHZ:
> +		reg = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void exynos4210_isol(struct samsung_usb2_phy_instance *inst,
> bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> +		break;
> +	case EXYNOS4210_HOST:
> +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_HOST;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask);
> +}
> +
> +static void exynos4210_phy_pwr(struct samsung_usb2_phy_instance *inst,
> bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> +		break;
> +	case EXYNOS4210_HOST:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> +				EXYNOS_4210_URSTCON_PHY1_P0 |
> +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> +		break;
> +	case EXYNOS4210_HSIC0:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> +		break;
> +	case EXYNOS4210_HSIC1:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk_reg_val, drv->reg_phy +
> EXYNOS_4210_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4210_power_on(struct samsung_usb2_phy_instance *inst)
> +{
> +	/* Order of initialisation is important - first power then
> isolation */
> +	exynos4210_phy_pwr(inst, 1);
> +	exynos4210_isol(inst, 0);
> +
> +	return 0;
> +}
> +
> +static int exynos4210_power_off(struct samsung_usb2_phy_instance *inst)
> +{
> +	exynos4210_isol(inst, 1);
> +	exynos4210_phy_pwr(inst, 0);
> +
> +	return 0;
> +}
> +
> +
> +static const struct samsung_usb2_common_phy exynos4210_phys[] = {
> +	{
> +		.label		= "device",
> +		.id		= EXYNOS4210_DEVICE,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.id		= EXYNOS4210_HOST,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.id		= EXYNOS4210_HSIC0,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.id		= EXYNOS4210_HSIC1,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{},
> +};
> +
> +const struct samsung_usb2_phy_config exynos4210_usb2_phy_config = {
> +	.num_phys		= EXYNOS4210_NUM_PHYS,
> +	.phys			= exynos4210_phys,
> +	.has_mode_switch	= 1,
> +};
> +
> diff --git a/drivers/phy/phy-exynos4212-usb2.c b/drivers/phy/phy-
> exynos4212-usb2.c
> new file mode 100644
> index 0000000..375ece0
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4212-usb2.c
> @@ -0,0 +1,312 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver - Exynos 4212 support
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/regmap.h>
> +#include <linux/spinlock.h>
> +#include "phy-samsung-usb2.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4212_UPHYPWR			0x0
> +
> +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)
> +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> +#define EXYNOS_4212_UPHYPWR_DEV	( \
> +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> +#define EXYNOS_4212_UPHYPWR_HOST ( \
> +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4212_UPHYCLK			0x4
> +
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> +
> +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> +
> +/* PHY reset control */
> +#define EXYNOS_4212_UPHYRST			0x8
> +
> +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4212_USB_ISOL_OFFSET		0x704
> +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x708
> +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x70c
> +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> +
> +/* Mode switching SUB Device <-> Host */
> +#define EXYNOS_4212_MODE_SWITCH_OFFSET		0x21c
> +#define EXYNOS_4212_MODE_SWITCH_MASK		1
> +#define EXYNOS_4212_MODE_SWITCH_DEVICE		0
> +#define EXYNOS_4212_MODE_SWITCH_HOST		1
> +
> +enum exynos4x12_phy_id {
> +	EXYNOS4212_DEVICE,
> +	EXYNOS4212_HOST,
> +	EXYNOS4212_HSIC0,
> +	EXYNOS4212_HSIC1,
> +	EXYNOS4212_NUM_PHYS,
> +};
> +
> +/*
> + * exynos4212_rate_to_clk() converts the supplied clock rate to the
> value that
> + * can be written to the phy register.
> + */
> +static int exynos4212_rate_to_clk(unsigned long rate, u32 *reg)
> +{
> +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> +
> +	switch (rate) {
> +	case 9600 * KHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> +		break;
> +	case 10 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> +		break;
> +	case 12 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 19200 * KHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> +		break;
> +	case 20 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> +		break;
> +	case 24 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 50 * MHZ:
> +		*reg = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void exynos4212_isol(struct samsung_usb2_phy_instance *inst,
> bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +	case EXYNOS4212_HOST:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask);
> +}
> +
> +static void exynos4212_phy_pwr(struct samsung_usb2_phy_instance *inst,
> bool on)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> +		break;
> +	case EXYNOS4212_HOST:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> +				EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk_reg_val, drv->reg_phy +
> EXYNOS_4212_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4212_power_on(struct samsung_usb2_phy_instance *inst)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +
> +	inst->enabled = 1;
> +	exynos4212_phy_pwr(inst, 1);
> +	exynos4212_isol(inst, 0);
> +
> +	/* Power on the device, as it is necessary for HSIC to work */
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct samsung_usb2_phy_instance *device =
> +					&drv->instances[EXYNOS4212_DEVICE];
> +		exynos4212_phy_pwr(device, 1);
> +		exynos4212_isol(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int exynos4212_power_off(struct samsung_usb2_phy_instance *inst)
> +{
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	struct samsung_usb2_phy_instance *device = &drv-
> >instances[EXYNOS4212_DEVICE];
> +
> +	inst->enabled = 0;
> +	exynos4212_isol(inst, 1);
> +	exynos4212_phy_pwr(inst, 0);
> +
> +	if (inst->cfg->id == EXYNOS4212_HSIC0 && !device->enabled) {
> +		exynos4212_isol(device, 1);
> +		exynos4212_phy_pwr(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static const struct samsung_usb2_common_phy exynos4212_phys[] = {
> +	{
> +		.label		= "device",
> +		.id		= EXYNOS4212_DEVICE,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.id		= EXYNOS4212_HOST,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.id		= EXYNOS4212_HSIC0,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.id		= EXYNOS4212_HSIC1,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{},
> +};
> +
> +const struct samsung_usb2_phy_config exynos4212_usb2_phy_config = {
> +	.num_phys		= EXYNOS4212_NUM_PHYS,
> +	.phys			= exynos4212_phys,
> +	.has_mode_switch	= 1,
> +};
> +
> diff --git a/drivers/phy/phy-samsung-usb2.c b/drivers/phy/phy-samsung-
> usb2.c
> new file mode 100644
> index 0000000..804ec77
> --- /dev/null
> +++ b/drivers/phy/phy-samsung-usb2.c
> @@ -0,0 +1,228 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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/mfd/syscon.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/spinlock.h>
> +#include "phy-samsung-usb2.h"
> +
> +static int samsung_usb2_phy_power_on(struct phy *phy)
> +{
> +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_dbg(drv->dev, "Request to power_on \"%s\" usb phy\n",
> +							inst->cfg->label);
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		goto err_main_clk;
> +	ret = clk_prepare_enable(inst->clk);
> +	if (ret)
> +		goto err_instance_clk;
> +	inst->rate = clk_get_rate(inst->clk);
> +	if (inst->cfg->rate_to_clk) {
> +		ret = inst->cfg->rate_to_clk(inst->rate, &inst-
> >clk_reg_val);
> +		if (ret)
> +			goto err_get_rate;
> +	}
> +	if (inst->cfg->power_on) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_on(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +
> +	return 0;
> +
> +err_get_rate:
> +	clk_disable_unprepare(inst->clk);
> +err_instance_clk:
> +	clk_disable_unprepare(drv->clk);
> +err_main_clk:
> +	return ret;
> +}
> +
> +static int samsung_usb2_phy_power_off(struct phy *phy)
> +{
> +	struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy);
> +	struct samsung_usb2_phy_driver *drv = inst->drv;
> +	int ret = 0;
> +
> +	dev_dbg(drv->dev, "Request to power_off \"%s\" usb phy\n",
> +							inst->cfg->label);
> +	if (inst->cfg->power_off) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_off(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(inst->clk);
> +	clk_disable_unprepare(drv->clk);
> +	return ret;
> +}
> +
> +static struct phy_ops samsung_usb2_phy_ops = {
> +	.power_on	= samsung_usb2_phy_power_on,
> +	.power_off	= samsung_usb2_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *samsung_usb2_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct samsung_usb2_phy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->instances[args->args[0]].phy;
> +}
> +
> +static const struct of_device_id samsung_usb2_phy_of_match[] = {
> +#ifdef CONFIG_PHY_EXYNOS4210_USB2
> +	{
> +		.compatible = "samsung,exynos4210-usb2-phy",
> +		.data = &exynos4210_usb2_phy_config,
> +	},
> +#endif
> +#ifdef CONFIG_PHY_EXYNOS4212_USB2
> +	{
> +		.compatible = "samsung,exynos4212-usb2-phy",
> +		.data = &exynos4212_usb2_phy_config,
> +	},
> +#endif
> +	{ },
> +};
> +
> +static int samsung_usb2_phy_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct samsung_usb2_phy_config *cfg;
> +	struct clk *clk;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct resource *mem;
> +	struct samsung_usb2_phy_driver *drv;
> +	int i;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(dev, "This driver is required to be instantiated
> from device tree\n");
> +		return -EINVAL;
> +	}
> +
> +	match = of_match_node(samsung_usb2_phy_of_match, pdev-
> >dev.of_node);
> +	if (!match) {
> +		dev_err(dev, "of_match_node() failed\n");
> +		return -EINVAL;
> +	}
> +	cfg = match->data;
> +
> +	drv = devm_kzalloc(dev, sizeof(struct samsung_usb2_phy_driver) +
> +		cfg->num_phys * sizeof(struct samsung_usb2_phy_instance),
> GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, drv);
> +	spin_lock_init(&drv->lock);
> +
> +	drv->cfg = cfg;
> +	drv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_phy)) {
> +		dev_err(dev, "Failed to map register memory (phy)\n");
> +		return PTR_ERR(drv->reg_phy);
> +	}
> +
> +	drv->reg_pmu = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +		"samsung,pmureg-phandle");
> +	if (IS_ERR(drv->reg_pmu)) {
> +		dev_err(dev, "Failed to map PMU registers (via syscon)\n");
> +		return PTR_ERR(drv->reg_pmu);
> +	}
> +
> +	if (drv->cfg->has_mode_switch) {
> +		drv->reg_sys = syscon_regmap_lookup_by_phandle(
> +				pdev->dev.of_node,
"samsung,sysreg-phandle");
> +		if (IS_ERR(drv->reg_sys)) {
> +			dev_err(dev, "Failed to map system registers (via
> syscon)\n");
> +			return PTR_ERR(drv->reg_sys);
> +		}
> +	}
> +
> +	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);
> +	}
> +
> +	for (i = 0; i < drv->cfg->num_phys; i++) {
> +		char *label = drv->cfg->phys[i].label;
> +		struct samsung_usb2_phy_instance *p = &drv->instances[i];
> +
> +		dev_dbg(dev, "Creating phy \"%s\"\n", label);
> +		p->phy = devm_phy_create(dev, &samsung_usb2_phy_ops, NULL);
> +		if (IS_ERR(p->phy)) {
> +			dev_err(drv->dev, "Failed to create usb2_phy
> \"%s\"\n",
> +						label);
> +			return PTR_ERR(p->phy);
> +		}
> +
> +		p->cfg = &drv->cfg->phys[i];
> +		p->drv = drv;
> +		phy_set_drvdata(p->phy, p);
> +
> +		clk = devm_clk_get(dev, p->cfg->label);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> +
p->cfg->label);
> +			return PTR_ERR(clk);
> +		}
> +		p->clk = clk;
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev,
> +
samsung_usb2_phy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(drv->dev, "Failed to register phy provider\n");
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver samsung_usb2_phy_driver = {
> +	.probe	= samsung_usb2_phy_probe,
> +	.driver = {
> +		.of_match_table	= samsung_usb2_phy_of_match,
> +		.name		= "samsung-usb2-phy",
> +		.owner		= THIS_MODULE,
> +	}
> +};
> +
> +module_platform_driver(samsung_usb2_phy_driver);
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> +MODULE_AUTHOR("Kamil Debski <k.debski@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:samsung-usb2-phy");
> +
> diff --git a/drivers/phy/phy-samsung-usb2.h b/drivers/phy/phy-samsung-
> usb2.h
> new file mode 100644
> index 0000000..cd12477
> --- /dev/null
> +++ b/drivers/phy/phy-samsung-usb2.h
> @@ -0,0 +1,72 @@
> +/*
> + * Samsung SoC USB 1.1/2.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@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.
> + */
> +
> +#ifndef _PHY_EXYNOS_USB2_H
> +#define _PHY_EXYNOS_USB2_H
> +
> +#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/spinlock.h>
> +
> +#define KHZ 1000
> +#define MHZ (KHZ * KHZ)
> +
> +struct samsung_usb2_phy_driver;
> +struct samsung_usb2_phy_instance;
> +struct samsung_usb2_phy_config;
> +
> +struct samsung_usb2_phy_instance {
> +	struct samsung_usb2_phy_driver *drv;
> +	struct phy *phy;
> +	const struct samsung_usb2_common_phy *cfg;
> +	char enabled;
> +	struct clk *clk;
> +	u32 clk_reg_val;
> +	unsigned long rate;
> +};
> +
> +struct samsung_usb2_phy_driver {
> +	struct device *dev;
> +	spinlock_t lock;
> +	void __iomem *reg_phy;
> +	struct regmap *reg_sys;
> +	struct regmap *reg_pmu;
> +	const struct samsung_usb2_phy_config *cfg;
> +	struct clk *clk;
> +	struct samsung_usb2_phy_instance instances[0];
> +};
> +
> +struct samsung_usb2_common_phy {
> +	char *label;
> +	unsigned int id;
> +	int (*rate_to_clk)(unsigned long, u32 *);
> +	int (*power_on)(struct samsung_usb2_phy_instance *);
> +	int (*power_off)(struct samsung_usb2_phy_instance *);
> +};
> +
> +
> +struct samsung_usb2_phy_config {
> +	int num_phys;
> +	const struct samsung_usb2_common_phy *phys;
> +	char has_mode_switch;
> +};
> +
> +extern const struct samsung_usb2_phy_config exynos4210_usb2_phy_config;
> +extern const struct samsung_usb2_phy_config exynos4212_usb2_phy_config;
> +#endif
> +
> --
> 1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Anton,

> From: Anton Tikhomirov [mailto:av.tikhomirov@samsung.com]
> Sent: Tuesday, December 10, 2013 3:43 AM
> 
> Hi Kamil,
> 
> Same USB2.0 PHY may be used by several HCDs, for example EHCI and OHCI.
> Consider the situation, when EHCI stops using the PHY and calls
> power_off, then OHCI becomes non-operational. In other words, PHY
> power_on and power_off calls must be balanced.
> 
> Shall we handle it in your driver? (usage count?)

Please look in the drivers/phy/phy-core.c file. Usage count is handled
there - see phy_power_on and phy_power_off functions. I understand that
after both EHCI and OHCI power on the phy, the usage count is 2. So
powering off one of them (EHCI for instance) the usage count is still
1, so the OHCI should still work properly.

[snip]

Best wishes,

Hi Kamil,

> Hi Anton,
> 
> > From: Anton Tikhomirov [mailto:av.tikhomirov@samsung.com]
> > Sent: Tuesday, December 10, 2013 3:43 AM
> >
> > Hi Kamil,
> >
> > Same USB2.0 PHY may be used by several HCDs, for example EHCI and
> OHCI.
> > Consider the situation, when EHCI stops using the PHY and calls
> > power_off, then OHCI becomes non-operational. In other words, PHY
> > power_on and power_off calls must be balanced.
> >
> > Shall we handle it in your driver? (usage count?)
> 
> Please look in the drivers/phy/phy-core.c file. Usage count is handled
> there - see phy_power_on and phy_power_off functions. I understand that
> after both EHCI and OHCI power on the phy, the usage count is 2. So
> powering off one of them (EHCI for instance) the usage count is still
> 1, so the OHCI should still work properly.

Oops, sorry, missed that.

> 
> [snip]
> 
> Best wishes,
> --
> Kamil Debski
> Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html