RE: [PATCH v2 1/5] phy: Add new Exynos USB PHY driver
From: Kamil Debski
Date: Mon Oct 28 2013 - 09:52:31 EST
Hi Kishon,
Thank you for your review! I will answer your comments below.
> From: Kishon Vijay Abraham I [mailto:kishon@xxxxxx]
> Sent: Friday, October 25, 2013 5:40 PM
>
> Hi,
>
> On Friday 25 October 2013 07:45 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@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> > .../devicetree/bindings/phy/samsung-usbphy.txt | 51 +++
> > drivers/phy/Kconfig | 21 ++
> > drivers/phy/Makefile | 3 +
> > drivers/phy/phy-exynos-usb.c | 245
> ++++++++++++++
> > drivers/phy/phy-exynos-usb.h | 94 ++++++
> > drivers/phy/phy-exynos4210-usb.c | 295
> +++++++++++++++++
> > drivers/phy/phy-exynos4212-usb.c | 343
> ++++++++++++++++++++
> > 7 files changed, 1052 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > create mode 100644 drivers/phy/phy-exynos-usb.c create mode 100644
> > drivers/phy/phy-exynos-usb.h create mode 100644
> > drivers/phy/phy-exynos4210-usb.c create mode 100644
> > drivers/phy/phy-exynos4212-usb.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > new file mode 100644
> > index 0000000..f112b37
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> > @@ -0,0 +1,51 @@
> > +Samsung S5P/EXYNOS SoC series USB PHY
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : should be one of the listed compatibles:
> > + - "samsung,exynos4210-usbphy"
> > + - "samsung,exynos4212-usbphy"
> > +- reg : a list of registers used by phy driver
> > + - first and obligatory is the location of phy modules registers
> > + - second and also required is the location of isolation registers
> > + (isolation registers control the physical connection between
> the in
> > + SoC modules and outside of the SoC, this also can be called
> enable
> > + control in the documentation of the SoC)
> > + - third is the location of the mode switch register, this only
> applies
> > + to SoCs that have such a feature; mode switching enables to
> have
> > + both host and device used the same SoC pins and is commonly
> used
> > + when OTG is supported
> > +- #phy-cells : from the generic phy bindings, must be 1;
> > +
> > +The second cell in the PHY specifier identifies the PHY its meaning
> > +is SoC dependent. For the currently supported SoCs (Exynos 4210 and
> > +Exynos 4212) it is as follows:
> > + 0 - USB device,
> > + 1 - USB host,
> > + 2 - HSIC0,
> > + 3 - HSIC1,
>
> HSIC is supposedly to be transceiver less no? You have to program
> something in the digital side?
> You have a single IP that have all these functionalities?
There is a single USB PHY controller for all the above functionalities
(i.e. host, device, hsic 0 and 1).
> > +
> > +Example:
> > +
> > +For Exynos 4412 (compatible with Exynos 4212):
> > +
> > +exynos_usbphy: exynos-usbphy@125B0000 {
> > + compatible = "samsung,exynos4212-usbphy";
> > + reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> > + ranges;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
>
> The above 3 properties aren't documented? Are they needed here?
My bad. I was working on two branches and corrected it in only one
of them.
> > + clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> > + <&clock 2>;
> > + clock-names = "phy", "device", "host", "hsic0", "hsic1";
> > + status = "okay";
> > + #phy-cells = <1>;
> > +};
> > +
> > +Then the PHY can be used in other nodes such as:
> > +
> > +ehci@12580000 {
> > + status = "okay";
> > + phys = <&exynos_usbphy 2>;
> > + phy-names = "hsic0";
> > +};
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
> > 349bef2..2f7ac0a 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -15,4 +15,25 @@ config GENERIC_PHY
> > phy users can obtain reference to the PHY. All the users of
> this
> > framework should select this config.
> >
> > +config PHY_EXYNOS_USB
> > + tristate "Samsung USB PHY driver (using the Generic PHY
> Framework)"
> Mentioning *Generic PHY Framework* is not necessary.
> *select GENERIC_PHY* here
This was added to distinguish this driver from the ols USB PHY driver.
I agree that in the final version it should be removed. I understand that
the correct way to do this is by removing the old driver when the new gets
merged. Yes?
> > + 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.
>
> If it's going to be used only for usb2, name it PHY_EXYNOS_USB2.
I agree.
> > +
> > +config PHY_EXYNOS4210_USB
> > + bool "Support for Exynos 4210"
> > + depends on PHY_EXYNOS_USB
> > + depends on CPU_EXYNOS4210
> > + help
> > + Enable USB PHY support for Exynos 4210
> > +
> > +config PHY_EXYNOS4212_USB
> > + bool "Support for Exynos 4212"
> > + depends on PHY_EXYNOS_USB
> > + depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> > + help
> > + Enable USB PHY support for Exynos 4212
>
> How difference is USB PHY in Exynos 4212 from Exynos 4210? If th
ere
> isn't much, I would suggest to use a single driver.
My idea for the driver is to have a single driver for the whole Exynos USB2
PHY. The core of the driver is in phy-exynos-usb.c and phy-exynos*-usb.c
provide registers and setup sequences for particular SoC versions.
There are a few differences between Exynos 4210, 4212, 5250 and S5PV210:
- the registers are different on each
- the setup sequence is different
- 4212 and 5250 have a single pair of regular USB OTG DEVICE/HOST lines
- 4210 has a USB OTG and a separate USB HOST lines
- S5PV210 has both USB OTG and a separate USB HOST but lacks any HSIC
interfaces
I agree with you that it is best to add as little code as possible.
Hence the idea to have a separate driver core and additional files for
distinctive SoCs. Enabling PHY_EXYNOS4210_USB (or other SoC specific
options)
does not add another driver it only includes support for enabled SoC.
It would be possible to skip this distinction altogether and include support
for all SoCs in the driver without config options.
> > +
> > endmenu
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
> > 9e9560f..ca3dc82 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -3,3 +3,6 @@
> > #
> >
> > obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> > +obj-$(CONFIG_PHY_EXYNOS_USB) += phy-exynos-usb.o
> > +obj-$(CONFIG_PHY_EXYNOS4210_USB) += phy-exynos4210-usb.o
> > +obj-$(CONFIG_PHY_EXYNOS4212_USB) += phy-exynos4212-usb.o
> > diff --git a/drivers/phy/phy-exynos-usb.c
> > b/drivers/phy/phy-exynos-usb.c new file mode 100644 index
> > 0000000..d4a26df
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos-usb.c
>
> phy-exynos-usb2.c?
Ok.
> > @@ -0,0 +1,245 @@
> > +/*
> > + * Samsung S5P/EXYNOS SoC series USB PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx>
> > + *
> > + * 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/spinlock.h>
> > +#include "phy-exynos-usb.h"
> > +
> > +static int exynos_uphy_power_on(struct phy *phy)
>
> exynos_usb2_phy here and everywhere below.
Ok.
> > +{
> > + struct uphy_instance *inst = phy_get_drvdata(phy);
> > + struct uphy_driver *drv = inst->drv;
> > + int ret;
> > +
> > + dev_info(drv->dev, "Request to power_on \"%s\" usb phy\n",
> > + inst->cfg->label);
>
> make it dev_dbg if it's necessary.
Ok.
> > + ret = clk_prepare_enable(drv->clk);
> > + if (ret)
> > + return ret;
> > + if (inst->cfg->power_on) {
> > + spin_lock(&drv->lock);
> > + ret = inst->cfg->power_on(inst);
> > + spin_unlock(&drv->lock);
> > + }
> > + clk_disable_unprepare(drv->clk);
>
> hmm.. don't you need the clock to be on during the duration of the PHY
> operation?
I think that it is enough to have the clock enabled only during changes.
> > + return ret;
> > +}
> > +
> > +static int exynos_uphy_power_off(struct phy *phy) {
> > + struct uphy_instance *inst = phy_get_drvdata(phy);
> > + struct uphy_driver *drv = inst->drv;
> > + int ret;
> > +
> > + dev_info(drv->dev, "Request to power_off \"%s\" usb phy\n",
> > + inst->cfg->label);
>
> dev_dbg?
Ok.
> > + ret = clk_prepare_enable(drv->clk);
> > + if (ret)
> > + return ret;
> > + if (inst->cfg->power_off) {
> > + spin_lock(&drv->lock);
> > + ret = inst->cfg->power_off(inst);
> > + spin_unlock(&drv->lock);
> > + }
> > + clk_disable_unprepare(drv->clk);
> > + return ret;
> > +}
> > +
> > +static struct phy_ops exynos_uphy_ops = {
> > + .power_on = exynos_uphy_power_on,
> > + .power_off = exynos_uphy_power_off,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static struct phy *exynos_uphy_xlate(struct device *dev,
> > + struct of_phandle_args *args)
> > +{
> > + struct uphy_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->uphy_instances[args->args[0]].phy;
> > +}
> > +
> > +static const struct of_device_id exynos_uphy_of_match[];
> > +
> > +static int exynos_uphy_probe(struct platform_device *pdev) {
> > + struct uphy_driver *drv;
> > + struct device *dev = &pdev->dev;
> > + struct resource *mem;
> > + struct phy_provider *phy_provider;
> > +
> > + const struct of_device_id *match;
> > + const struct uphy_config *cfg;
> > + struct clk *clk;
> > +
> > + int i;
> > +
> > + match = of_match_node(exynos_uphy_of_match, pdev->dev.of_node);
> > + if (!match) {
> > + dev_err(dev, "of_match_node() failed\n");
> > + return -EINVAL;
> > + }
> > + cfg = match->data;
> > + if (!cfg) {
> > + dev_err(dev, "Failed to get configuration\n");
> > + return -EINVAL;
> > + }
> > +
> > + drv = devm_kzalloc(dev, sizeof(struct uphy_driver) +
> > + cfg->num_phys * sizeof(struct uphy_instance), GFP_KERNEL);
> > +
> > + if (!drv) {
> > + dev_err(dev, "Failed to allocate memory\n");
> > + 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);
> > +
> empty blank line.
Will fix.
> > + 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);
> > + }
> > +
> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + drv->reg_isol = devm_ioremap_resource(dev, mem);
> > + if (IS_ERR(drv->reg_isol)) {
> > + dev_err(dev, "Failed to map register memory (isolation)\n");
> > + return PTR_ERR(drv->reg_isol);
> > + }
> > +
> > + switch (drv->cfg->cpu) {
> > + case TYPE_EXYNOS4210:
> > + case TYPE_EXYNOS4212:
>
> Lets not add such cpu checks inside driver.
Some SoC have a special register, which switches the OTG lines between
device and host modes. I understand that it might not be the prettiest
code. I see this as a good compromise between having a single huge
driver for all Exynos SoCs and having a multiple drivers for each SoC
version. PHY IPs in these chips very are similar but have to be handled
differently. Any other ideas to solve this issue?
> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > + drv->reg_mode = devm_ioremap_resource(dev, mem);
> > + if (IS_ERR(drv->reg_mode)) {
> > + dev_err(dev, "Failed to map register memory (mode
> switch)\n");
> > + return PTR_ERR(drv->reg_mode);
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + phy_provider = devm_of_phy_provider_register(dev,
> > + exynos_uphy_xlate);
> > + if (IS_ERR(phy_provider)) {
> > + dev_err(drv->dev, "Failed to register phy provider\n");
> > + return PTR_ERR(phy_provider);
> > + }
> > +
> > + 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 uphy_instance *p = &drv->uphy_instances[i];
> > +
> > + dev_info(dev, "Creating phy \"%s\"\n", label);
> > + p->phy = devm_phy_create(dev, &exynos_uphy_ops, NULL);
> > + if (IS_ERR(p->phy)) {
> > + dev_err(drv->dev, "Failed to create uphy \"%s\"\n",
> > + label);
> > + return PTR_ERR(p->phy);
> > + }
> > +
> > + p->cfg = &drv->cfg->phys[i];
> > + p->drv = drv;
> > + phy_set_drvdata(p->phy, p);
> > +
> > + clk = 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->rate = clk_get_rate(clk);
> > +
> > + if (p->cfg->rate_to_clk) {
> > + p->clk = p->cfg->rate_to_clk(p->rate);
> > + if (p->clk == CLKSEL_ERROR) {
> > + dev_err(dev, "Clock rate (%ld) not
supported\n",
> > + p->rate);
> > + clk_put(clk);
> > + return -EINVAL;
> > + }
> > + }
> > + clk_put(clk);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PHY_EXYNOS4210_USB
> Do we really need this?
No we don't. The driver can always support all Exynos SoC versions. These
config options were added for flexibility.
>
> > +extern const struct uphy_config exynos4210_uphy_config; #endif
> > +
> > +#ifdef CONFIG_PHY_EXYNOS4212_USB
>
> Same here.
> > +extern const struct uphy_config exynos4212_uphy_config; #endif
> > +
> > +static const struct of_device_id exynos_uphy_of_match[] = { #ifdef
> > +CONFIG_PHY_EXYNOS4210_USB
>
> #if not needed here.
If the #ifdef CONFIG_PHY_EXYNOS4210_USB is removed then yes. Otherwise
it is necessary - exynos4210_uphy_config may be undefined.
> > + {
> > + .compatible = "samsung,exynos4210-usbphy",
> > + .data = &exynos4210_uphy_config,
> > + },
> > +#endif
> > +#ifdef CONFIG_PHY_EXYNOS4212_USB
>
> here too.
> > + {
> > + .compatible = "samsung,exynos4212-usbphy",
> > + .data = &exynos4212_uphy_config,
> > + },
> > +#endif
> > + { },
> > +};
> > +
> > +static struct platform_driver exynos_uphy_driver = {
> > + .probe = exynos_uphy_probe,
> > + .driver = {
> > + .of_match_table = exynos_uphy_of_match,
> > + .name = "exynos-usbphy-new",
> "exynos-usb2-phy".
> > + .owner = THIS_MODULE,
> > + }
> > +};
> > +
> > +module_platform_driver(exynos_uphy_driver);
> > +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> > +MODULE_AUTHOR("Kamil Debski <k.debski@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:exynos-uphy-new");
> > +
> > diff --git a/drivers/phy/phy-exynos-usb.h
> > b/drivers/phy/phy-exynos-usb.h new file mode 100644 index
> > 0000000..f45cb3c
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos-usb.h
> > @@ -0,0 +1,94 @@
> > +/*
> > + * Samsung S5P/EXYNOS SoC series USB PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx>
> > + *
> > + * 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_SAMSUNG_NEW_H
> > +#define _PHY_SAMSUNG_NEW_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 CLKSEL_ERROR -1
> > +
> > +#ifndef KHZ
> > +#define KHZ 1000
> > +#endif
> > +
> > +#ifndef MHZ
> > +#define MHZ (KHZ * KHZ)
> > +#endif
> > +
> > +enum phy_type {
> > + PHY_DEVICE,
> > + PHY_HOST,
> > +};
> > +
> > +enum samsung_cpu_type {
> > + TYPE_S3C64XX,
> > + TYPE_EXYNOS4210,
> > + TYPE_EXYNOS4212,
>
> No *cpu_type* inside driver files.
I guess that you are in favor a "a separate driver for each phy version".
For me it can be both. But we have to discuss which apporach is better:
1) separate driver for each phy version - no iffs and significant code
duplication
2) a single driver driver supporting all Exynos variants - it needs ifs,
code is always bigger
3) a single driver with support for particular SoC enabled in the config
file
- with ifs, but the driver can be compiled smaller
> > +};
> > +
> > +enum uphy_state {
> > + STATE_OFF,
> > + STATE_ON,
> > +};
> > +
> > +struct uphy_driver;
> > +struct uphy_instance;
> > +struct uphy_config;
> > +
> > +struct uphy_instance {
> > + struct uphy_driver *drv;
> > + struct phy *phy;
> > + const struct common_phy *cfg;
> > + enum uphy_state state;
> > + int ref_cnt;
> > + u32 clk;
> > + unsigned long rate;
> > +};
> > +
> > +struct uphy_driver {
> > + struct device *dev;
> > + spinlock_t lock;
> > + void __iomem *reg_phy;
> > + void __iomem *reg_isol;
> > + void __iomem *reg_mode;
> > + const struct uphy_config *cfg;
> > + struct clk *clk;
> > + struct uphy_instance uphy_instances[0];
>
> you might have more than one phy instance right?
Yes. The USB PHY IP controls USB device, host and hsics.
> > +};
> > +
> > +struct common_phy {
> > + char *label;
> > + enum phy_type type;
> > + unsigned int id;
> > + u32 (*rate_to_clk)(unsigned long);
> > + int (*power_on)(struct uphy_instance*);
> > + int (*power_off)(struct uphy_instance*); };
> > +
> > +
> > +struct uphy_config {
> > + enum samsung_cpu_type cpu;
> > + int num_phys;
> > + const struct common_phy *phys;
> > +};
> > +
> > +#endif
> > +
> > diff --git a/drivers/phy/phy-exynos4210-usb.c
> > b/drivers/phy/phy-exynos4210-usb.c
> > new file mode 100644
> > index 0000000..6cf74f7
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos4210-usb.c
> > @@ -0,0 +1,295 @@
> > +/*
> > + * Samsung S5P/EXYNOS SoC series USB PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx>
> > + *
> > + * 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/spinlock.h>
> > +#include "phy-exynos-usb.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)But
> > +
> > +/* Isolation, configured in the power management unit */
> > +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET 0x0
> > +#define EXYNOS_4210_USB_ISOL_DEVICE (1 << 0)
> > +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET 0x4
> > +#define EXYNOS_4210_USB_ISOL_HOST (1 << 0)
> > +
> > +/* USBYPHY1 Floating prevention */
> > +#define EXYNOS_4210_UPHY1CON 0x34
> > +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION 0x1
> > +
> > +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. */
>
> use proper commenting styles Documentation/CodingStyle
Ok, I will.
> > +static u32 exynos4210_rate_to_clk(unsigned long rate) {
> > + unsigned int clksel;
> > +
> > + switch (rate) {
> > + case 12 * MHZ:
> > + clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> > + break;
> > + case 24 * MHZ:
> > + clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> > + break;
> > + case 48 * MHZ:
> > + clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> > + break;
> > + default:
> > + clksel = CLKSEL_ERROR;
> > + }
> > +
> > + return clksel;
> > +}
> > +
> > +static void exynos4210_isol(struct uphy_instance *inst, bool on) {
> > + struct uphy_driver *drv = inst->drv;
> > + u32 offset;
> > + u32 mask;
> > + u32 tmp;
> > +
> > + if (!drv->reg_isol)
> > + return;
> > +
> > + 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;
> > + };
> > +
> > + tmp = readl(drv->reg_isol + offset);
> > + if (on)
> > + tmp &= ~mask;
> > + else
> > + tmp |= mask;
> > + writel(tmp, drv->reg_isol + offset); }
> > +
> > +static void exynos4210_phy_pwr(struct uphy_instance *inst, bool on)
> {
> > + struct uphy_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, 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);
>
> usleep_range?
Maybe. The above code is based on the drivers/usb/phy/phy-samsung-usb2.c.
My documentation does not specify the acceptable delay range.
> > + 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 uphy_instance *inst) {
> > + struct uphy_driver *drv = inst->drv;
> > +
> > + if (inst->state == STATE_ON) {
> > + dev_err(drv->dev, "usb phy \"%s\" already on",
> > + inst->cfg->label);
> > + return -ENODEV;
> > + }
> > + inst->state = STATE_ON;
> > + inst->ref_cnt++;
> > + if (inst->ref_cnt > 1)
> > + return 0;
>
> reference counting shouldn't be needed. It's been handled by the PHY
> framework.
I will try how it works without this. There was some code that was removed
and
used the reference counter. I had some trouble figuring out the correct
setup
sequence, when multiple phys were enabled (e.g. only host, host + device,
only
device).
> > +
> > + /* 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 uphy_instance *inst) {
> > + struct uphy_driver *drv = inst->drv;
> > +
> > + if (inst->state == STATE_OFF) {
> > + dev_err(drv->dev, "usb phy \"%s\" already off",
> > + inst->cfg->label);
> > + return -EINVAL;
> > + }
> > +
> > + inst->state = STATE_OFF;
> > + inst->ref_cnt--;
> > + if (inst->ref_cnt > 0)
> > + return 0;
>
> same here.
> > +
> > + exynos4210_isol(inst, 1);
> > + exynos4210_phy_pwr(inst, 0);
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static const struct common_phy exynos4210_phys[] = {
> > + {
> > + .label = "device",
> > + .type = PHY_DEVICE,
> > + .id = EXYNOS4210_DEVICE,
> > + .rate_to_clk = exynos4210_rate_to_clk,
> > + .power_on = exynos4210_power_on,
> > + .power_off = exynos4210_power_off,
> > + },
> > + {
> > + .label = "host",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4210_HOST,
> > + .rate_to_clk = exynos4210_rate_to_clk,
> > + .power_on = exynos4210_power_on,
> > + .power_off = exynos4210_power_off,
> > + },
> > + {
> > + .label = "hsic0",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4210_HSIC0,
> > + .rate_to_clk = exynos4210_rate_to_clk,
> > + .power_on = exynos4210_power_on,
> > + .power_off = exynos4210_power_off,
> > + },
> > + {
> > + .label = "hsic1",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4210_HSIC1,
> > + .rate_to_clk = exynos4210_rate_to_clk,
> > + .power_on = exynos4210_power_on,
> > + .power_off = exynos4210_power_off,
> > + },
> > + {},
> > +};
> > +
> > +const struct uphy_config exynos4210_uphy_config = {
> > + .cpu = TYPE_EXYNOS4210,
> > + .num_phys = EXYNOS4210_NUM_PHYS,
> > + .phys = exynos4210_phys,
> > +};
> > +
> > diff --git a/drivers/phy/phy-exynos4212-usb.c
> > b/drivers/phy/phy-exynos4212-usb.c
> > new file mode 100644
> > index 0000000..c07ae8e
> > --- /dev/null
> > +++ b/drivers/phy/phy-exynos4212-usb.c
> > @@ -0,0 +1,343 @@
> > +/*
> > + * Samsung S5P/EXYNOS SoC series USB PHY driver
> > + *
> > + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx>
> > + *
> > + * 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/spinlock.h>
> > +#include "phy-exynos-usb.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 0x0
> > +#define EXYNOS_4212_USB_ISOL_OTG (1 << 0)
> > +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET 0x4
> > +#define EXYNOS_4212_USB_ISOL_HSIC0 (1 << 0)
> > +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET 0x8
> > +#define EXYNOS_4212_USB_ISOL_HSIC1 (1 << 0)
> > +
> > +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 u32
> > +exynos4212_rate_to_clk(unsigned long rate) {
> > + unsigned int clksel;
> > +
> > + /* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> > +
> > + switch (rate) {
> > + case 9600 * KHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> > + break;
> > + case 10 * MHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> > + break;
> > + case 12 * MHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> > + break;
> > + case 19200 * KHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> > + break;
> > + case 20 * MHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> > + break;
> > + case 24 * MHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> > + break;
> > + case 50 * MHZ:
> > + clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> > + break;
> > + default:
> > + clksel = CLKSEL_ERROR;
> > + }
> > +
> > + return clksel;
> > +}
> > +
> > +static void exynos4212_isol(struct uphy_instance *inst, bool on) {
> > + struct uphy_driver *drv = inst->drv;
> > + u32 offset;
> > + u32 mask;
> > + u32 tmp;
> > +
> > + if (!drv->reg_isol)
> > + return;
> > +
> > + switch (inst->cfg->id) {
> > + case EXYNOS4212_DEVICE:
> > + offset = EXYNOS_4212_USB_ISOL_OFFSET;
> > + mask = EXYNOS_4212_USB_ISOL_OTG;
> > + break;
> > + 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;
> > + };
> > +
> > + tmp = readl(drv->reg_isol + offset);
> > + if (on)
> > + tmp &= ~mask;
> > + else
> > + tmp |= mask;
> > + writel(tmp, drv->reg_isol + offset); }
> > +
> > +static void exynos4212_phy_pwr(struct uphy_instance *inst, bool on)
> {
> > + struct uphy_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, 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 uphy_instance *inst) {
> > + struct uphy_driver *drv = inst->drv;
> > +
> > + if (inst->state == STATE_ON) {
> > + dev_err(drv->dev, "usb phy \"%s\" already on",
> > + inst->cfg->label);
> > + return -ENODEV;
> > + }
> > +
> > + inst->state = STATE_ON;
> > + inst->ref_cnt++;
> > + if (inst->ref_cnt > 1)
> > + return 0;
> > +
> > + 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 uphy_instance *device =
> > +
&drv->uphy_instances[EXYNOS4212_DEVICE];
> > + exynos4212_phy_pwr(device, 1);
> > + exynos4212_isol(device, 0);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int exynos4212_power_off(struct uphy_instance *inst) {
> > + struct uphy_driver *drv = inst->drv;
> > +
> > + if (inst->state == STATE_OFF) {
> > + dev_err(drv->dev, "usb phy \"%s\" already off",
> > + inst->cfg->label);
> > + return -EINVAL;
> > + }
> > +
> > + inst->state = STATE_OFF;
> > + inst->ref_cnt--;
> > +
> > + if (inst->ref_cnt > 0)
> > + return 0;
> > +
> > + exynos4212_isol(inst, 1);
> > + exynos4212_phy_pwr(inst, 0);
> > +
> > + if (inst->cfg->id == EXYNOS4212_HSIC0) {
> > + struct uphy_instance *device =
> > +
&drv->uphy_instances[EXYNOS4212_DEVICE];
> > + exynos4212_isol(device, 1);
> > + exynos4212_phy_pwr(device, 0);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static const struct common_phy exynos4212_phys[] = {
> > + {
> > + .label = "device",
> > + .type = PHY_DEVICE,
> > + .id = EXYNOS4212_DEVICE,
> > + .rate_to_clk = exynos4212_rate_to_clk,
> > + .power_on = exynos4212_power_on,
> > + .power_off = exynos4212_power_off,
> > + },
> > + {
> > + .label = "host",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4212_HOST,
> > + .rate_to_clk = exynos4212_rate_to_clk,
> > + .power_on = exynos4212_power_on,
> > + .power_off = exynos4212_power_off,
> > + },
> > + {
> > + .label = "hsic0",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4212_HSIC0,
> > + .rate_to_clk = exynos4212_rate_to_clk,
> > + .power_on = exynos4212_power_on,
> > + .power_off = exynos4212_power_off,
> > + },
> > + {
> > + .label = "hsic1",
> > + .type = PHY_HOST,
> > + .id = EXYNOS4212_HSIC1,
> > + .rate_to_clk = exynos4212_rate_to_clk,
> > + .power_on = exynos4212_power_on,
> > + .power_off = exynos4212_power_off,
> > + },
> > + {},
> > +};
> > +
> > +const struct uphy_config exynos4212_uphy_config = {
> > + .cpu = TYPE_EXYNOS4212,
> > + .num_phys = EXYNOS4212_NUM_PHYS,
> > + .phys = exynos4212_phys,
> > +};
>
> I see quite an amount of similarities between the two PHY drivers. It
> would be better if we can have a single driver to handle it.
Maybe my description was confusing. This is a single driver with the option
to compile in or leave out support for particular SoCs. It is not a problem
to have all compiled in, maybe it will be clearer that way. The problem is
that
the registers are different for particular SoCs and the initialization
procedures differ a bit.
>
> Thanks
> Kishon
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/