| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <00fc01cef2a0$263acd00$72b06700$%debski@samsung.com>
Date: Fri, 06 Dec 2013 17:28:07 +0100
From: Kamil Debski <k.debski@...sung.com>
To: 'Kishon Vijay Abraham I' <kishon@...com>,
linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
balbi@...com, 'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>
Cc: kyungmin.park@...sung.com, Tomasz Figa <t.figa@...sung.com>,
Sylwester Nawrocki <s.nawrocki@...sung.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
gautam.vivek@...sung.com, mat.krawczuk@...il.com,
yulgon.kim@...sung.com, p.paneri@...sung.com,
av.tikhomirov@...sung.com, jg1.han@...sung.com,
galak@...eaurora.org, matt.porter@...aro.org
Subject: RE: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi Kishon,
Thank you for the review.
> From: Kishon Vijay Abraham I [mailto:kishon@...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@...sung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@...sung.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@...B0000 {
>
> 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@...40000 {
> > + 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@...sung.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@...sung.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@...sung.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@...sung.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@...sung.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,
--
Kamil Debski
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists