| Message ID | 1361419646-9052-2-git-send-email-chao.xie@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 20, 2013 at 11:07:11PM -0500, Chao Xie wrote: > The PHY is seperated from usb controller. > The usb controller used in marvell pxa168/pxa910/mmp2 are same, > but PHY initialization may be different. > the usb controller can support udc/otg/ehci, and for each of > the mode, it need PHY to initialized before use the controller. > Direclty writing the phy driver will make the usb controller > driver to be simple and portable. > The PHY driver will be used by marvell udc/otg/ehci. > > Signed-off-by: Chao Xie <chao.xie@marvell.com> > --- > drivers/usb/phy/Kconfig | 7 + > drivers/usb/phy/Makefile | 1 + > drivers/usb/phy/mv_usb2_phy.c | 402 ++++++++++++++++++++++++++++++++++ > include/linux/platform_data/mv_usb.h | 9 +- > include/linux/usb/mv_usb2.h | 32 +++ > 5 files changed, 448 insertions(+), 3 deletions(-) > create mode 100644 drivers/usb/phy/mv_usb2_phy.c > create mode 100644 include/linux/usb/mv_usb2.h > > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig > index 65217a5..5479760 100644 > --- a/drivers/usb/phy/Kconfig > +++ b/drivers/usb/phy/Kconfig > @@ -73,3 +73,10 @@ config SAMSUNG_USBPHY > help > Enable this to support Samsung USB phy controller for samsung > SoCs. > + > +config MV_USB2_PHY > + tristate "Marvell USB 2.0 PHY Driver" > + depends on USB || USB_GADGET NAK, PHYs don't depend on the gadget framework. > + help > + Enable this to support Marvell USB 2.0 phy driver for Marvell > + SoC. > diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile > index b13faa1..3835316 100644 > --- a/drivers/usb/phy/Makefile > +++ b/drivers/usb/phy/Makefile > @@ -12,3 +12,4 @@ obj-$(CONFIG_MV_U3D_PHY) += mv_u3d_phy.o > obj-$(CONFIG_USB_EHCI_TEGRA) += tegra_usb_phy.o > obj-$(CONFIG_USB_RCAR_PHY) += rcar-phy.o > obj-$(CONFIG_SAMSUNG_USBPHY) += samsung-usbphy.o > +obj-$(CONFIG_MV_USB2_PHY) += mv_usb2_phy.o > diff --git a/drivers/usb/phy/mv_usb2_phy.c b/drivers/usb/phy/mv_usb2_phy.c > new file mode 100644 > index 0000000..a81e5e4 > --- /dev/null > +++ b/drivers/usb/phy/mv_usb2_phy.c > @@ -0,0 +1,402 @@ > +/* > + * Copyright (C) 2013 Marvell Inc. > + * > + * Author: > + * Chao Xie <xiechao.mail@gmail.com> > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/resource.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/clk.h> > +#include <linux/export.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/platform_data/mv_usb.h> > +#include <linux/usb/phy.h> > +#include <linux/usb/mv_usb2.h> > + > +/* phy regs */ > +#define UTMI_REVISION 0x0 > +#define UTMI_CTRL 0x4 > +#define UTMI_PLL 0x8 > +#define UTMI_TX 0xc > +#define UTMI_RX 0x10 > +#define UTMI_IVREF 0x14 > +#define UTMI_T0 0x18 > +#define UTMI_T1 0x1c > +#define UTMI_T2 0x20 > +#define UTMI_T3 0x24 > +#define UTMI_T4 0x28 > +#define UTMI_T5 0x2c > +#define UTMI_RESERVE 0x30 > +#define UTMI_USB_INT 0x34 > +#define UTMI_DBG_CTL 0x38 > +#define UTMI_OTG_ADDON 0x3c prepend these with MV_USB_ > +enum mv_usb2_phy_type { > + PXA168_USB, > + PXA910_USB, > + MMP2_USB, > +}; > + > +static unsigned int u2o_get(void __iomem *base, unsigned int offset) > +{ > + return readl(base + offset); > +} > + > +static void u2o_set(void __iomem *base, unsigned int offset, > + unsigned int value) > +{ > + u32 reg; > + > + reg = readl(base + offset); > + reg |= value; > + writel(reg, base + offset); > + /* > + * read after write. It will make sure writing takes effect. > + * It is suggested by PHY design engineer. > + */ > + readl(base + offset); ewww... you really don't need (and *shouldn't* use) u2o_set() or u2o_clear(). They clearly prevent compiler from optimizing variable usage and could cause pressure on your interconnect. By writing and reading multiple times for no reason. > +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy) > +{ > + struct platform_device *pdev = mv_phy->pdev; > + unsigned int loops = 0; > + void __iomem *base = mv_phy->base; > + > + dev_dbg(&pdev->dev, "phy init\n"); remove this debugging message. > + /* Initialize the USB PHY power */ > + if (mv_phy->type == PXA910_USB) { you _do_ have a REVISION register. Are you 100% certain that revision is the same on all your devices ? It seems to me that you should be doing proper revision detection instead of adding the hacky enumeration of yours. > + /* UTMI_IVREF */ > + if (mv_phy->type == PXA168_USB) > + /* fixing Microsoft Altair board interface with NEC hub issue - > + * Set UTMI_IVREF from 0x4a3 to 0x4bf */ wrong comment style. Run *ALL* your patches through checkpatch.pl --strict and sparse. > + u2o_write(base, UTMI_IVREF, 0x4bf); > + > + /* toggle VCOCAL_START bit of UTMI_PLL */ > + udelay(200); why the udelay() calls ? Add a comment to code explaining them. > + u2o_set(base, UTMI_PLL, VCOCAL_START); > + udelay(40); > + u2o_clear(base, UTMI_PLL, VCOCAL_START); > + > + /* toggle REG_RCAL_START bit of UTMI_TX */ > + udelay(400); > + u2o_set(base, UTMI_TX, REG_RCAL_START); > + udelay(40); > + u2o_clear(base, UTMI_TX, REG_RCAL_START); > + udelay(400); > + > + /* Make sure PHY PLL is ready */ > + loops = 0; > + while ((u2o_get(base, UTMI_PLL) & PLL_READY) == 0) { > + mdelay(1); > + loops++; > + if (loops > 100) { > + dev_warn(&pdev->dev, "calibrate timeout, UTMI_PLL %x\n", > + u2o_get(base, UTMI_PLL)); if this fails, shouldn't you return an error code ? > +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy) > +{ > + void __iomem *base = mv_phy->base; > + > + if (mv_phy->type == PXA168_USB) > + u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON); > + > + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN); > + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN); > + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN); > + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); > + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); NAK, this is stupid, read once, clear bits you want to clear and write only once. > + return 0; > +} > + > +static int mv_usb2_phy_init(struct usb_phy *phy) > +{ > + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); > + int i = 0; > + > + mutex_lock(&mv_phy->phy_lock); what's this mutex for ? > + if (mv_phy->refcount++ == 0) { can this device really be used simultaneously by multiple devices ? > + for (i = 0; i < mv_phy->clks_num; i++) { > + mv_phy->clks[i] = devm_clk_get(&pdev->dev, > + pdata->clkname[i]); *NEVER* pass clock names via platform_data, this is utterly wrong. > + if (IS_ERR(mv_phy->clks[i])) { > + dev_err(&pdev->dev, "failed to get clock %s\n", > + pdata->clkname[i]); > + return PTR_ERR(mv_phy->clks[i]); > + } > + } > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (r == NULL) { > + dev_err(&pdev->dev, "no phy I/O memory resource defined\n"); > + return -ENODEV; > + } > + mv_phy->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); use devm_ioremap_resource() instead. > +static struct platform_driver mv_usb2_phy_driver = { > + .probe = mv_usb2_phy_probe, > + .remove = mv_usb2_phy_remove, > + .driver = { > + .name = "pxa168-usb-phy", > + }, > + .id_table = mv_usb2_phy_ids, > +}; > + > +static int __init mv_usb2_phy_driver_init(void) > +{ > + return platform_driver_register(&mv_usb2_phy_driver); > +} > +arch_initcall(mv_usb2_phy_driver_init); NAK, use module_platform_driver() like everybody else and handle registration ordering with -EPROBE_DEFER. > diff --git a/include/linux/platform_data/mv_usb.h b/include/linux/platform_data/mv_usb.h > index 944b01d..fd3d1b4 100644 > --- a/include/linux/platform_data/mv_usb.h > +++ b/include/linux/platform_data/mv_usb.h > @@ -47,9 +47,12 @@ struct mv_usb_platform_data { > /* Force a_bus_req to be asserted */ > unsigned int otg_force_a_bus_req:1; > > - int (*phy_init)(void __iomem *regbase); > - void (*phy_deinit)(void __iomem *regbase); > int (*set_vbus)(unsigned int vbus); > - int (*private_init)(void __iomem *opregs, void __iomem *phyregs); should be part of a separate patch. > }; > + > +struct mv_usb_phy_platform_data { > + unsigned int clknum; > + char **clkname; > +}; NAK for this platform_data.
On Mon, Mar 4, 2013 at 10:21 PM, Felipe Balbi <balbi@ti.com> wrote: > On Wed, Feb 20, 2013 at 11:07:11PM -0500, Chao Xie wrote: >> The PHY is seperated from usb controller. >> The usb controller used in marvell pxa168/pxa910/mmp2 are same, >> but PHY initialization may be different. >> the usb controller can support udc/otg/ehci, and for each of >> the mode, it need PHY to initialized before use the controller. >> Direclty writing the phy driver will make the usb controller >> driver to be simple and portable. >> The PHY driver will be used by marvell udc/otg/ehci. >> >> Signed-off-by: Chao Xie <chao.xie@marvell.com> >> --- >> drivers/usb/phy/Kconfig | 7 + >> drivers/usb/phy/Makefile | 1 + >> drivers/usb/phy/mv_usb2_phy.c | 402 ++++++++++++++++++++++++++++++++++ >> include/linux/platform_data/mv_usb.h | 9 +- >> include/linux/usb/mv_usb2.h | 32 +++ >> 5 files changed, 448 insertions(+), 3 deletions(-) >> create mode 100644 drivers/usb/phy/mv_usb2_phy.c >> create mode 100644 include/linux/usb/mv_usb2.h >> >> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig >> index 65217a5..5479760 100644 >> --- a/drivers/usb/phy/Kconfig >> +++ b/drivers/usb/phy/Kconfig >> @@ -73,3 +73,10 @@ config SAMSUNG_USBPHY >> help >> Enable this to support Samsung USB phy controller for samsung >> SoCs. >> + >> +config MV_USB2_PHY >> + tristate "Marvell USB 2.0 PHY Driver" >> + depends on USB || USB_GADGET > > NAK, PHYs don't depend on the gadget framework. Sure, i will remove it. > >> + help >> + Enable this to support Marvell USB 2.0 phy driver for Marvell >> + SoC. >> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile >> index b13faa1..3835316 100644 >> --- a/drivers/usb/phy/Makefile >> +++ b/drivers/usb/phy/Makefile >> @@ -12,3 +12,4 @@ obj-$(CONFIG_MV_U3D_PHY) += mv_u3d_phy.o >> obj-$(CONFIG_USB_EHCI_TEGRA) += tegra_usb_phy.o >> obj-$(CONFIG_USB_RCAR_PHY) += rcar-phy.o >> obj-$(CONFIG_SAMSUNG_USBPHY) += samsung-usbphy.o >> +obj-$(CONFIG_MV_USB2_PHY) += mv_usb2_phy.o >> diff --git a/drivers/usb/phy/mv_usb2_phy.c b/drivers/usb/phy/mv_usb2_phy.c >> new file mode 100644 >> index 0000000..a81e5e4 >> --- /dev/null >> +++ b/drivers/usb/phy/mv_usb2_phy.c >> @@ -0,0 +1,402 @@ >> +/* >> + * Copyright (C) 2013 Marvell Inc. >> + * >> + * Author: >> + * Chao Xie <xiechao.mail@gmail.com> >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> >> +#define UTMI_CTRL 0x4 >> +#define UTMI_PLL 0x8 >> +#define UTMI_TX 0xc >> +#define UTMI_RX 0x10 >> +#define UTMI_IVREF 0x14 >> +#define UTMI_T0 0x18 >> +#define UTMI_T1 0x1c >> +#define UTMI_T2 0x20 >> +#define UTMI_T3 0x24 >> +#define UTMI_T4 0x28 >> +#define UTMI_T5 0x2c >> +#define UTMI_RESERVE 0x30 >> +#define UTMI_USB_INT 0x34 >> +#define UTMI_DBG_CTL 0x38 >> +#define UTMI_OTG_ADDON 0x3c > > prepend these with MV_USB_ Fine. > >> +enum mv_usb2_phy_type { >> + PXA168_USB, >> + PXA910_USB, >> + MMP2_USB, >> +}; > > > ewww... you really don't need (and *shouldn't* use) u2o_set() or > u2o_clear(). They clearly prevent compiler from optimizing variable > usage and could cause pressure on your interconnect. By writing and > reading multiple times for no reason. > The APIs defined here is for device operation. The device register read/write is not same as memory. When a silicon comes, it may not be stable, and will have done some workaround epecially for the device register read/write. to define the APIs for the register read/write will help to implement the workaround without changing phy init code every time. Now the only constrain is should read back the registers if you have writen to it. It will low down the performance, but it does not matter. Because phy init will only done once when you initialize it. I will think about reove the u2o_xxx APIs. >> +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy) >> +{ >> + struct platform_device *pdev = mv_phy->pdev; >> + unsigned int loops = 0; >> + void __iomem *base = mv_phy->base; >> + >> + dev_dbg(&pdev->dev, "phy init\n"); > > remove this debugging message. > >> + /* Initialize the USB PHY power */ >> + if (mv_phy->type == PXA910_USB) { > > you _do_ have a REVISION register. Are you 100% certain that revision is > the same on all your devices ? It seems to me that you should be doing > proper revision detection instead of adding the hacky enumeration of > yours. We do not have revison registers and will do not want ot define #ifdef CPU_PXA910 or CPU_PXA_XXX or use is_cpu_pxa910 or cpu_pxa_xxx because it is not acceptable. for example, if we have new SOC and it use the same PHY as pxa910 i have to change the USB driver code to support it. for example #ifdef CPU_PXA910 || CPU_XXX So i have to depends on the device_id to do the work. > >> + /* UTMI_IVREF */ >> + if (mv_phy->type == PXA168_USB) >> + /* fixing Microsoft Altair board interface with NEC hub issue - >> + * Set UTMI_IVREF from 0x4a3 to 0x4bf */ > > wrong comment style. Run *ALL* your patches through checkpatch.pl > --strict and sparse. > It seems that checkpatch.pl can not detect everything. I really use checpatch.pl for every patch i sent to maillist. sorry for that, i will fix it. >> + u2o_write(base, UTMI_IVREF, 0x4bf); >> + >> + /* toggle VCOCAL_START bit of UTMI_PLL */ >> + udelay(200); > > why the udelay() calls ? Add a comment to code explaining them. > >> + u2o_set(base, UTMI_PLL, VCOCAL_START); >> + udelay(40); >> + u2o_clear(base, UTMI_PLL, VCOCAL_START); >> + >> + /* toggle REG_RCAL_START bit of UTMI_TX */ >> + udelay(400); >> + u2o_set(base, UTMI_TX, REG_RCAL_START); >> + udelay(40); >> + u2o_clear(base, UTMI_TX, REG_RCAL_START); >> + udelay(400); >> + >> + /* Make sure PHY PLL is ready */ >> + loops = 0; >> + while ((u2o_get(base, UTMI_PLL) & PLL_READY) == 0) { >> + mdelay(1); >> + loops++; >> + if (loops > 100) { >> + dev_warn(&pdev->dev, "calibrate timeout, UTMI_PLL %x\n", >> + u2o_get(base, UTMI_PLL)); > > if this fails, shouldn't you return an error code ? > yes. it should return the error code. >> +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy) >> +{ >> + void __iomem *base = mv_phy->base; >> + >> + if (mv_phy->type == PXA168_USB) >> + u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON); >> + >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN); >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN); >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN); >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); > > NAK, this is stupid, read once, clear bits you want to clear and write > only once. > I will check with silicon design engineer. becuase all the phy init code is delivered by them. I can not tell that if there are any speciall reason they will do so because the device register read/write is not same as normal memory. >> + return 0; >> +} >> + >> +static int mv_usb2_phy_init(struct usb_phy *phy) >> +{ >> + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); >> + int i = 0; >> + >> + mutex_lock(&mv_phy->phy_lock); > > what's this mutex for ? > >> + if (mv_phy->refcount++ == 0) { > > can this device really be used simultaneously by multiple devices ? > Sure, we have another EHCI device, it will share the PHY with this USB controller. So we will use refcount and mutext to protect the phy init. >> + for (i = 0; i < mv_phy->clks_num; i++) { >> + mv_phy->clks[i] = devm_clk_get(&pdev->dev, >> + pdata->clkname[i]); > > *NEVER* pass clock names via platform_data, this is utterly wrong. > without device tree support, the only way we can get the clock is the pdata. the use phy have mutiple clocks. So what do you suggest to handle it? >> + if (IS_ERR(mv_phy->clks[i])) { >> + dev_err(&pdev->dev, "failed to get clock %s\n", >> + pdata->clkname[i]); >> + return PTR_ERR(mv_phy->clks[i]); >> + } >> + } >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (r == NULL) { >> + dev_err(&pdev->dev, "no phy I/O memory resource defined\n"); >> + return -ENODEV; >> + } >> + mv_phy->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > > use devm_ioremap_resource() instead. > fine. >> +static struct platform_driver mv_usb2_phy_driver = { >> + .probe = mv_usb2_phy_probe, >> + .remove = mv_usb2_phy_remove, >> + .driver = { >> + .name = "pxa168-usb-phy", >> + }, >> + .id_table = mv_usb2_phy_ids, >> +}; >> + >> +static int __init mv_usb2_phy_driver_init(void) >> +{ >> + return platform_driver_register(&mv_usb2_phy_driver); >> +} >> +arch_initcall(mv_usb2_phy_driver_init); > > NAK, use module_platform_driver() like everybody else and handle > registration ordering with -EPROBE_DEFER. > The reason i do not use module_platform_driver is the compiling sequence of each directory of driver/usb/ the phy is compiled after otg/ehci. So it means that it can not find the usb phy, and will register fail. >> diff --git a/include/linux/platform_data/mv_usb.h b/include/linux/platform_data/mv_usb.h >> index 944b01d..fd3d1b4 100644 >> --- a/include/linux/platform_data/mv_usb.h >> +++ b/include/linux/platform_data/mv_usb.h >> @@ -47,9 +47,12 @@ struct mv_usb_platform_data { >> /* Force a_bus_req to be asserted */ >> unsigned int otg_force_a_bus_req:1; >> >> - int (*phy_init)(void __iomem *regbase); >> - void (*phy_deinit)(void __iomem *regbase); >> int (*set_vbus)(unsigned int vbus); >> - int (*private_init)(void __iomem *opregs, void __iomem *phyregs); > > should be part of a separate patch. > >> }; >> + >> +struct mv_usb_phy_platform_data { >> + unsigned int clknum; >> + char **clkname; >> +}; > > NAK for this platform_data. > > -- > balbi
Hi, On Tue, Mar 05, 2013 at 10:03:01AM +0800, Chao Xie wrote: > >> +enum mv_usb2_phy_type { > >> + PXA168_USB, > >> + PXA910_USB, > >> + MMP2_USB, > >> +}; > > > > > > ewww... you really don't need (and *shouldn't* use) u2o_set() or > > u2o_clear(). They clearly prevent compiler from optimizing variable > > usage and could cause pressure on your interconnect. By writing and > > reading multiple times for no reason. > > > > The APIs defined here is for device operation. The device register > read/write is not same as memory. > When a silicon comes, it may not be stable, and will have done some > workaround epecially for the device register read/write. to define the > APIs for the register read/write will help to implement the workaround > without changing phy init code every time. > Now the only constrain is should read back the registers if you have > writen to it. > It will low down the performance, but it does not matter. Because phy > init will only done once when you initialize it. > I will think about reove the u2o_xxx APIs. You didn't even understand what I meant. Seriously. Anyway, details are as follows: readl() and writel() always add a memory barrier around each operation. This is good because it makes sure memory mapped I/O region is always ordered, but your current usage will post reads and writes on the interconnect over and over again for no reason. What you should do, with any register access is: reg = readl(); reg &= ~BITS_TO_DISABLE; reg |= BITS_TO_ENABLE; writel(reg); Whereas your current code does: reg = readl(); reg &= ~BITS_TO_DISABLE; writel(reg); reg = readl(); reg |= BITS_TO_ENABLE; writel(reg); You do that for each bit, in some cases. > >> +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy) > >> +{ > >> + struct platform_device *pdev = mv_phy->pdev; > >> + unsigned int loops = 0; > >> + void __iomem *base = mv_phy->base; > >> + > >> + dev_dbg(&pdev->dev, "phy init\n"); > > > > remove this debugging message. > > > >> + /* Initialize the USB PHY power */ > >> + if (mv_phy->type == PXA910_USB) { > > > > you _do_ have a REVISION register. Are you 100% certain that revision is > > the same on all your devices ? It seems to me that you should be doing > > proper revision detection instead of adding the hacky enumeration of > > yours. > > We do not have revison registers and will do not want ot define > #ifdef CPU_PXA910 or CPU_PXA_XXX > or > use is_cpu_pxa910 or cpu_pxa_xxx > because it is not acceptable. for example, if we have new SOC and it > use the same PHY as pxa910 > i have to change the USB driver code to support it. for example > #ifdef CPU_PXA910 || CPU_XXX what a load of crap. *read your code!!!* There is a UTMI_REVISION register defined at offset 0. > So i have to depends on the device_id to do the work. > > > >> + /* UTMI_IVREF */ > >> + if (mv_phy->type == PXA168_USB) > >> + /* fixing Microsoft Altair board interface with NEC hub issue - > >> + * Set UTMI_IVREF from 0x4a3 to 0x4bf */ > > > > wrong comment style. Run *ALL* your patches through checkpatch.pl > > --strict and sparse. > > > > It seems that checkpatch.pl can not detect everything. I really use > checpatch.pl for > every patch i sent to maillist. > sorry for that, i will fix it. did you use --strict ? > >> +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy) > >> +{ > >> + void __iomem *base = mv_phy->base; > >> + > >> + if (mv_phy->type == PXA168_USB) > >> + u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON); > >> + > >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN); > >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN); > >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN); > >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); > >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); > > > > NAK, this is stupid, read once, clear bits you want to clear and write > > only once. > > > > I will check with silicon design engineer. becuase all the phy init > code is delivered by them. right, but you need to be critical with any code you read. You can't simply blindly make it compile on linux and hope that it'll work efficiently. I doubt your silicon validation evironment has support for the memory barriers which are added on each readl()/writel() calls. > I can not tell that if there are any speciall reason they will do so > because the device register read/write is not same as normal memory. seriously ? Read your documentation dude, there's nothing that will require you to enable one bit in the register, then flush the write, then write another bit, flush the write, write another bit, flush the write and so on. Those cases are extremely rare (MUSB has only *ONE* such case with regards to DMA_MODE bit) and in 99% of the cases, you can write everything to a variable and post a single write to your interconnect. Stop trying to come up with nonsensical excuses that "register read/write is not same as normal memory", of course it's not same as normal memory, and that's why we have standardized I/O functions. > >> + return 0; > >> +} > >> + > >> +static int mv_usb2_phy_init(struct usb_phy *phy) > >> +{ > >> + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); > >> + int i = 0; > >> + > >> + mutex_lock(&mv_phy->phy_lock); > > > > what's this mutex for ? > > > >> + if (mv_phy->refcount++ == 0) { > > > > can this device really be used simultaneously by multiple devices ? > > > > Sure, we have another EHCI device, it will share the PHY with this USB > controller. what is "this USB controller", current driver is for the PHY only. Do you mean to say that PHY will be shared by EHCI and UDC ? > So we will use refcount and mutext to protect the phy init. in that case, it might be better to add a generic refcounting to the PHY layer as a whole, instead of cooking your own private solutions. > >> + for (i = 0; i < mv_phy->clks_num; i++) { > >> + mv_phy->clks[i] = devm_clk_get(&pdev->dev, > >> + pdata->clkname[i]); > > > > *NEVER* pass clock names via platform_data, this is utterly wrong. > > > without device tree support, the only way we can get the clock is the pdata. > the use phy have mutiple clocks. > So what do you suggest to handle it? clkdev > >> +static struct platform_driver mv_usb2_phy_driver = { > >> + .probe = mv_usb2_phy_probe, > >> + .remove = mv_usb2_phy_remove, > >> + .driver = { > >> + .name = "pxa168-usb-phy", > >> + }, > >> + .id_table = mv_usb2_phy_ids, > >> +}; > >> + > >> +static int __init mv_usb2_phy_driver_init(void) > >> +{ > >> + return platform_driver_register(&mv_usb2_phy_driver); > >> +} > >> +arch_initcall(mv_usb2_phy_driver_init); > > > > NAK, use module_platform_driver() like everybody else and handle > > registration ordering with -EPROBE_DEFER. > > > > The reason i do not use module_platform_driver is the compiling > sequence of each directory of driver/usb/ > the phy is compiled after otg/ehci. So it means that it can not find > the usb phy, and will register fail. it doesn't matter, you should teach EHCI about -EPROBE_DEFER. If it can't grab the PHY, return -EPROBE_DEFER.
On Tue, 5 Mar 2013, Felipe Balbi wrote: > Anyway, details are as follows: > > readl() and writel() always add a memory barrier around each operation. Is that supposed to be true on all architectures or only on ARM? Alan Stern
On Tue, Mar 05, 2013 at 11:43:35AM -0500, Alan Stern wrote: > On Tue, 5 Mar 2013, Felipe Balbi wrote: > > > Anyway, details are as follows: > > > > readl() and writel() always add a memory barrier around each operation. > > Is that supposed to be true on all architectures or only on ARM? I believe it's true for every architecture which doesn't have strongly ordered memory accesses. ARM is just one example of that ;-) In any case, his read->set 1 bit->write loop isn't good even for architectures with strongly ordered memory accesses. It will continue to post unnecessary reads and writes to the interconnect.
On Tue, Mar 5, 2013 at 7:04 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Tue, Mar 05, 2013 at 10:03:01AM +0800, Chao Xie wrote: >> >> +enum mv_usb2_phy_type { >> >> + PXA168_USB, >> >> + PXA910_USB, >> >> + MMP2_USB, >> >> +}; >> > >> > >> > ewww... you really don't need (and *shouldn't* use) u2o_set() or >> > u2o_clear(). They clearly prevent compiler from optimizing variable >> > usage and could cause pressure on your interconnect. By writing and >> > reading multiple times for no reason. >> > >> >> The APIs defined here is for device operation. The device register >> read/write is not same as memory. >> When a silicon comes, it may not be stable, and will have done some >> workaround epecially for the device register read/write. to define the >> APIs for the register read/write will help to implement the workaround >> without changing phy init code every time. >> Now the only constrain is should read back the registers if you have >> writen to it. >> It will low down the performance, but it does not matter. Because phy >> init will only done once when you initialize it. >> I will think about reove the u2o_xxx APIs. > > You didn't even understand what I meant. Seriously. > > Anyway, details are as follows: > > readl() and writel() always add a memory barrier around each operation. > > This is good because it makes sure memory mapped I/O region is always > ordered, but your current usage will post reads and writes on the > interconnect over and over again for no reason. What you should do, with > any register access is: > > reg = readl(); > > reg &= ~BITS_TO_DISABLE; > reg |= BITS_TO_ENABLE; > > writel(reg); > > Whereas your current code does: > > reg = readl(); > reg &= ~BITS_TO_DISABLE; > writel(reg); > > reg = readl(); > reg |= BITS_TO_ENABLE; > writel(reg); > > You do that for each bit, in some cases. > >> >> +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy) >> >> +{ >> >> + struct platform_device *pdev = mv_phy->pdev; >> >> + unsigned int loops = 0; >> >> + void __iomem *base = mv_phy->base; >> >> + >> >> + dev_dbg(&pdev->dev, "phy init\n"); >> > >> > remove this debugging message. >> > >> >> + /* Initialize the USB PHY power */ >> >> + if (mv_phy->type == PXA910_USB) { >> > >> > you _do_ have a REVISION register. Are you 100% certain that revision is >> > the same on all your devices ? It seems to me that you should be doing >> > proper revision detection instead of adding the hacky enumeration of >> > yours. >> >> We do not have revison registers and will do not want ot define >> #ifdef CPU_PXA910 or CPU_PXA_XXX >> or >> use is_cpu_pxa910 or cpu_pxa_xxx >> because it is not acceptable. for example, if we have new SOC and it >> use the same PHY as pxa910 >> i have to change the USB driver code to support it. for example >> #ifdef CPU_PXA910 || CPU_XXX > > what a load of crap. > > *read your code!!!* > > There is a UTMI_REVISION register defined at offset 0. > >> So i have to depends on the device_id to do the work. >> > >> >> + /* UTMI_IVREF */ >> >> + if (mv_phy->type == PXA168_USB) >> >> + /* fixing Microsoft Altair board interface with NEC hub issue - >> >> + * Set UTMI_IVREF from 0x4a3 to 0x4bf */ >> > >> > wrong comment style. Run *ALL* your patches through checkpatch.pl >> > --strict and sparse. >> > >> >> It seems that checkpatch.pl can not detect everything. I really use >> checpatch.pl for >> every patch i sent to maillist. >> sorry for that, i will fix it. > > did you use --strict ? > >> >> +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy) >> >> +{ >> >> + void __iomem *base = mv_phy->base; >> >> + >> >> + if (mv_phy->type == PXA168_USB) >> >> + u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON); >> >> + >> >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN); >> >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN); >> >> + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN); >> >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); >> >> + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); >> > >> > NAK, this is stupid, read once, clear bits you want to clear and write >> > only once. >> > >> >> I will check with silicon design engineer. becuase all the phy init >> code is delivered by them. > > right, but you need to be critical with any code you read. You can't > simply blindly make it compile on linux and hope that it'll work > efficiently. > > I doubt your silicon validation evironment has support for the memory > barriers which are added on each readl()/writel() calls. > >> I can not tell that if there are any speciall reason they will do so >> because the device register read/write is not same as normal memory. > > seriously ? Read your documentation dude, there's nothing that will > require you to enable one bit in the register, then flush the write, > then write another bit, flush the write, write another bit, flush the > write and so on. > > Those cases are extremely rare (MUSB has only *ONE* such case with > regards to DMA_MODE bit) and in 99% of the cases, you can write > everything to a variable and post a single write to your interconnect. > > Stop trying to come up with nonsensical excuses that "register > read/write is not same as normal memory", of course it's not same as > normal memory, and that's why we have standardized I/O functions. > >> >> + return 0; >> >> +} >> >> + >> >> +static int mv_usb2_phy_init(struct usb_phy *phy) >> >> +{ >> >> + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); >> >> + int i = 0; >> >> + >> >> + mutex_lock(&mv_phy->phy_lock); >> > >> > what's this mutex for ? >> > >> >> + if (mv_phy->refcount++ == 0) { >> > >> > can this device really be used simultaneously by multiple devices ? >> > >> >> Sure, we have another EHCI device, it will share the PHY with this USB >> controller. > > what is "this USB controller", current driver is for the PHY only. Do > you mean to say that PHY will be shared by EHCI and UDC ? > >> So we will use refcount and mutext to protect the phy init. > > in that case, it might be better to add a generic refcounting to the PHY > layer as a whole, instead of cooking your own private solutions. > >> >> + for (i = 0; i < mv_phy->clks_num; i++) { >> >> + mv_phy->clks[i] = devm_clk_get(&pdev->dev, >> >> + pdata->clkname[i]); >> > >> > *NEVER* pass clock names via platform_data, this is utterly wrong. >> > >> without device tree support, the only way we can get the clock is the pdata. >> the use phy have mutiple clocks. >> So what do you suggest to handle it? > > clkdev > >> >> +static struct platform_driver mv_usb2_phy_driver = { >> >> + .probe = mv_usb2_phy_probe, >> >> + .remove = mv_usb2_phy_remove, >> >> + .driver = { >> >> + .name = "pxa168-usb-phy", >> >> + }, >> >> + .id_table = mv_usb2_phy_ids, >> >> +}; >> >> + >> >> +static int __init mv_usb2_phy_driver_init(void) >> >> +{ >> >> + return platform_driver_register(&mv_usb2_phy_driver); >> >> +} >> >> +arch_initcall(mv_usb2_phy_driver_init); >> > >> > NAK, use module_platform_driver() like everybody else and handle >> > registration ordering with -EPROBE_DEFER. >> > >> >> The reason i do not use module_platform_driver is the compiling >> sequence of each directory of driver/usb/ >> the phy is compiled after otg/ehci. So it means that it can not find >> the usb phy, and will register fail. > > it doesn't matter, you should teach EHCI about -EPROBE_DEFER. If it > can't grab the PHY, return -EPROBE_DEFER. > > -- > balbi Thanks for the comments, i summary the feedbacks and suggestions as below, 1. For the phy setting and u2o_xx APIs something, i will optimize it. If there are some special cases, i will add comments. 2. For the module_platform_driver, -EPROBE_DEFER is what i missed. I will check it and thanks for suggestion. 3. For the revison register. It exists in some SOCes(pxa168), but for some SOCes, the register dispears(pxa910, armada610). These SOCes are developed by different desgin teams, and it need to be enhanced, but for current products i have to use the device_id to detect the PHY controller. 4. For the mutex and refcount. The "USB controller" includes two blocks - the udc and ehci. In fact they will not be used at same time, but for some SOCes it duplicates the ehci block. It means that the SOCes have two or more ehci blocks. The added ehci blocks depend on the PHY to be initialized, and they can be used at same time as the "USB controller". That is the reason i add mutex and refcount for phy driver. 5. For the clock name. Can you describe it in details? For clkdev, if it want to find the clock, it still depends on the dev_name and con_id.
Hi, On Wed, Mar 06, 2013 at 10:11:41AM +0800, Chao Xie wrote: > 3. For the revison register. It exists in some SOCes(pxa168), but for > some SOCes, the register dispears(pxa910, armada610). These SOCes are > developed by different desgin teams, and it need to be enhanced, but > for current products i have to use the device_id to detect the PHY > controller. fair enough, make sure to add a comment to the driver about this so grumpy maintainers stop complaining ;-) > 4. For the mutex and refcount. The "USB controller" includes two > blocks - the udc and ehci. In fact they will not be used at same time, > but for some SOCes it duplicates the ehci block. It means that the > SOCes have two or more ehci blocks. The added ehci blocks depend on > the PHY to be initialized, and they can be used at same time as the > "USB controller". That is the reason i add mutex and refcount for phy > driver. alright, let's add refcounting to the generic PHY layer then. > 5. For the clock name. Can you describe it in details? For clkdev, if > it want to find the clock, it still depends on the dev_name and > con_id. right, the idea of clkdev is that you associate a clock provider with its consumer by means of dev_name. Then you can fetch the clock from driver with any name you want. Which means if you do you clkdev properly you could: clk_get(dev, "clk1"); clk_get(dev, "clk2"); ... and so on. good luck
On Wed, Mar 6, 2013 at 4:10 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Mar 06, 2013 at 10:11:41AM +0800, Chao Xie wrote: >> 3. For the revison register. It exists in some SOCes(pxa168), but for >> some SOCes, the register dispears(pxa910, armada610). These SOCes are >> developed by different desgin teams, and it need to be enhanced, but >> for current products i have to use the device_id to detect the PHY >> controller. > > fair enough, make sure to add a comment to the driver about this so > grumpy maintainers stop complaining ;-) > >> 4. For the mutex and refcount. The "USB controller" includes two >> blocks - the udc and ehci. In fact they will not be used at same time, >> but for some SOCes it duplicates the ehci block. It means that the >> SOCes have two or more ehci blocks. The added ehci blocks depend on >> the PHY to be initialized, and they can be used at same time as the >> "USB controller". That is the reason i add mutex and refcount for phy >> driver. > > alright, let's add refcounting to the generic PHY layer then. > >> 5. For the clock name. Can you describe it in details? For clkdev, if >> it want to find the clock, it still depends on the dev_name and >> con_id. > > right, the idea of clkdev is that you associate a clock provider with > its consumer by means of dev_name. Then you can fetch the clock from > driver with any name you want. Which means if you do you clkdev properly > you could: > > clk_get(dev, "clk1"); > clk_get(dev, "clk2"); > ... > > and so on. > It is same as what i think. The clock numbers and names are depent of SOCes, i do not want to fix it in ther driver. So i use pdata to pass the clknum and clk names "clk1", "clk2" and so on. in the driver, i use devm_clk_get(&pdev->dev, pdata->clkname[i]); Do you think it is acceptable? When device tree for clock framework are supported, the things get easier, and it is my next step. > good luck > > -- > balbi
On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote: > On Wed, Mar 6, 2013 at 4:10 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Wed, Mar 06, 2013 at 10:11:41AM +0800, Chao Xie wrote: > >> 3. For the revison register. It exists in some SOCes(pxa168), but for > >> some SOCes, the register dispears(pxa910, armada610). These SOCes are > >> developed by different desgin teams, and it need to be enhanced, but > >> for current products i have to use the device_id to detect the PHY > >> controller. > > > > fair enough, make sure to add a comment to the driver about this so > > grumpy maintainers stop complaining ;-) > > > >> 4. For the mutex and refcount. The "USB controller" includes two > >> blocks - the udc and ehci. In fact they will not be used at same time, > >> but for some SOCes it duplicates the ehci block. It means that the > >> SOCes have two or more ehci blocks. The added ehci blocks depend on > >> the PHY to be initialized, and they can be used at same time as the > >> "USB controller". That is the reason i add mutex and refcount for phy > >> driver. > > > > alright, let's add refcounting to the generic PHY layer then. > > > >> 5. For the clock name. Can you describe it in details? For clkdev, if > >> it want to find the clock, it still depends on the dev_name and > >> con_id. > > > > right, the idea of clkdev is that you associate a clock provider with > > its consumer by means of dev_name. Then you can fetch the clock from > > driver with any name you want. Which means if you do you clkdev properly > > you could: > > > > clk_get(dev, "clk1"); > > clk_get(dev, "clk2"); > > ... > > > > and so on. > > > > It is same as what i think. > The clock numbers and names are depent of SOCes, i do not want to fix no, they're not dependent on anything, not if you use clkdev correctly. > it in ther driver. So i use pdata to pass the clknum and clk names > "clk1", "clk2" and so on. > in the driver, i use > devm_clk_get(&pdev->dev, pdata->clkname[i]); > Do you think it is acceptable? no > When device tree for clock framework are supported, the things get > easier, and it is my next step. not strong enough argument to accept passing clock names via platform_data.
On Wed, Mar 6, 2013 at 4:53 PM, Felipe Balbi <balbi@ti.com> wrote: > On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote: >> On Wed, Mar 6, 2013 at 4:10 PM, Felipe Balbi <balbi@ti.com> wrote: >> > Hi, >> > >> > On Wed, Mar 06, 2013 at 10:11:41AM +0800, Chao Xie wrote: >> >> 3. For the revison register. It exists in some SOCes(pxa168), but for >> >> some SOCes, the register dispears(pxa910, armada610). These SOCes are >> >> developed by different desgin teams, and it need to be enhanced, but >> >> for current products i have to use the device_id to detect the PHY >> >> controller. >> > >> > fair enough, make sure to add a comment to the driver about this so >> > grumpy maintainers stop complaining ;-) >> > >> >> 4. For the mutex and refcount. The "USB controller" includes two >> >> blocks - the udc and ehci. In fact they will not be used at same time, >> >> but for some SOCes it duplicates the ehci block. It means that the >> >> SOCes have two or more ehci blocks. The added ehci blocks depend on >> >> the PHY to be initialized, and they can be used at same time as the >> >> "USB controller". That is the reason i add mutex and refcount for phy >> >> driver. >> > >> > alright, let's add refcounting to the generic PHY layer then. >> > >> >> 5. For the clock name. Can you describe it in details? For clkdev, if >> >> it want to find the clock, it still depends on the dev_name and >> >> con_id. >> > >> > right, the idea of clkdev is that you associate a clock provider with >> > its consumer by means of dev_name. Then you can fetch the clock from >> > driver with any name you want. Which means if you do you clkdev properly >> > you could: >> > >> > clk_get(dev, "clk1"); >> > clk_get(dev, "clk2"); >> > ... >> > >> > and so on. >> > >> >> It is same as what i think. >> The clock numbers and names are depent of SOCes, i do not want to fix > > no, they're not dependent on anything, not if you use clkdev correctly. > So 1. Does it mean that all SOCes clock driver should define same names such as "clk1", "clk2"? 2. Does it mean that if driver failed at clk_get, the probing will not stop because this SOC may define this clock? >> it in ther driver. So i use pdata to pass the clknum and clk names >> "clk1", "clk2" and so on. >> in the driver, i use >> devm_clk_get(&pdev->dev, pdata->clkname[i]); >> Do you think it is acceptable? > > no > >> When device tree for clock framework are supported, the things get >> easier, and it is my next step. > > not strong enough argument to accept passing clock names via > platform_data. > > -- > balbi
On Wed, Mar 06, 2013 at 05:02:28PM +0800, Chao Xie wrote: > On Wed, Mar 6, 2013 at 4:53 PM, Felipe Balbi <balbi@ti.com> wrote: > > On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote: > >> On Wed, Mar 6, 2013 at 4:10 PM, Felipe Balbi <balbi@ti.com> wrote: > >> > Hi, > >> > > >> > On Wed, Mar 06, 2013 at 10:11:41AM +0800, Chao Xie wrote: > >> >> 3. For the revison register. It exists in some SOCes(pxa168), but for > >> >> some SOCes, the register dispears(pxa910, armada610). These SOCes are > >> >> developed by different desgin teams, and it need to be enhanced, but > >> >> for current products i have to use the device_id to detect the PHY > >> >> controller. > >> > > >> > fair enough, make sure to add a comment to the driver about this so > >> > grumpy maintainers stop complaining ;-) > >> > > >> >> 4. For the mutex and refcount. The "USB controller" includes two > >> >> blocks - the udc and ehci. In fact they will not be used at same time, > >> >> but for some SOCes it duplicates the ehci block. It means that the > >> >> SOCes have two or more ehci blocks. The added ehci blocks depend on > >> >> the PHY to be initialized, and they can be used at same time as the > >> >> "USB controller". That is the reason i add mutex and refcount for phy > >> >> driver. > >> > > >> > alright, let's add refcounting to the generic PHY layer then. > >> > > >> >> 5. For the clock name. Can you describe it in details? For clkdev, if > >> >> it want to find the clock, it still depends on the dev_name and > >> >> con_id. > >> > > >> > right, the idea of clkdev is that you associate a clock provider with > >> > its consumer by means of dev_name. Then you can fetch the clock from > >> > driver with any name you want. Which means if you do you clkdev properly > >> > you could: > >> > > >> > clk_get(dev, "clk1"); > >> > clk_get(dev, "clk2"); > >> > ... > >> > > >> > and so on. > >> > > >> > >> It is same as what i think. > >> The clock numbers and names are depent of SOCes, i do not want to fix > > > > no, they're not dependent on anything, not if you use clkdev correctly. > > > > So > 1. Does it mean that all SOCes clock driver should define same names > such as "clk1", "clk2"? clk1, clk2, clk3 were just examples. Ideally you would have more meaningfull names like "fck" for functional clock. > 2. Does it mean that if driver failed at clk_get, the probing will not > stop because this SOC may define this clock? that depends on how you write your driver. If platform A has 3 clocks and platform B has only 2 you have two options: a) make the third clock optional by not bailing out if clk_get() fails b) define a dummy no-op clock node to satisfy the third clock.
On Tue, Mar 05, 2013 at 10:03:01AM +0800, Chao Xie wrote: > On Mon, Mar 4, 2013 at 10:21 PM, Felipe Balbi <balbi@ti.com> wrote: > > On Wed, Feb 20, 2013 at 11:07:11PM -0500, Chao Xie wrote: > >> + for (i = 0; i < mv_phy->clks_num; i++) { > >> + mv_phy->clks[i] = devm_clk_get(&pdev->dev, > >> + pdata->clkname[i]); > > > > *NEVER* pass clock names via platform_data, this is utterly wrong. > > > without device tree support, the only way we can get the clock is the pdata. > the use phy have mutiple clocks. > So what do you suggest to handle it? Then you don't understand the clk API at all. Read the documentation in include/linux/clk.h for clk_get(). The first parameter is the device which you're interested in getting the clock for. The second parameter defines the INPUT as a string to THAT DEVICE. It is specific to the device. It is NOT the system name of the clock. So, if you have a function clock and an interface clock to a device, then use a name like "fck" for the function clock and "ick" for the interface clock. Do _NOT_ make the mistake of using "global" clock names. People have done that many times in the past and got into horrid sticky problems - and ended up with _far_ more code than is really necessary if you do things the right way.
On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote:
> The clock numbers and names are depent of SOCes,
No they aren't. The clock names used to describe them in your documentation
may vary, but their _purpose_ for the sake of the device will be fixed -
and you should name them appropriately from the _device_ point of view.
Not the SoC point of view. That way leads to total madness. We've
proven this over the years that we've had the clk API and people have
come up with trash implementations that do that crap. After many
years of struggling, they've seen the light and fixed their shite up
to work the way I originally intended, and... instantly benefited from
it.
On Thu, Mar 7, 2013 at 12:48 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote: >> The clock numbers and names are depent of SOCes, > > No they aren't. The clock names used to describe them in your documentation > may vary, but their _purpose_ for the sake of the device will be fixed - > and you should name them appropriately from the _device_ point of view. > > Not the SoC point of view. That way leads to total madness. We've > proven this over the years that we've had the clk API and people have > come up with trash implementations that do that crap. After many > years of struggling, they've seen the light and fixed their shite up > to work the way I originally intended, and... instantly benefited from > it. Yes, you are right, and you explained it very clearly.. I understand what you said. Our SOC document is lack of this kind of inoformation. I will talk with the desgin engneer to make clear it, and make a new version of patches. Thanks for your review and comments.
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig index 65217a5..5479760 100644 --- a/drivers/usb/phy/Kconfig +++ b/drivers/usb/phy/Kconfig @@ -73,3 +73,10 @@ config SAMSUNG_USBPHY help Enable this to support Samsung USB phy controller for samsung SoCs. + +config MV_USB2_PHY + tristate "Marvell USB 2.0 PHY Driver" + depends on USB || USB_GADGET + help + Enable this to support Marvell USB 2.0 phy driver for Marvell + SoC. diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile index b13faa1..3835316 100644 --- a/drivers/usb/phy/Makefile +++ b/drivers/usb/phy/Makefile @@ -12,3 +12,4 @@ obj-$(CONFIG_MV_U3D_PHY) += mv_u3d_phy.o obj-$(CONFIG_USB_EHCI_TEGRA) += tegra_usb_phy.o obj-$(CONFIG_USB_RCAR_PHY) += rcar-phy.o obj-$(CONFIG_SAMSUNG_USBPHY) += samsung-usbphy.o +obj-$(CONFIG_MV_USB2_PHY) += mv_usb2_phy.o diff --git a/drivers/usb/phy/mv_usb2_phy.c b/drivers/usb/phy/mv_usb2_phy.c new file mode 100644 index 0000000..a81e5e4 --- /dev/null +++ b/drivers/usb/phy/mv_usb2_phy.c @@ -0,0 +1,402 @@ +/* + * Copyright (C) 2013 Marvell Inc. + * + * Author: + * Chao Xie <xiechao.mail@gmail.com> + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/resource.h> +#include <linux/delay.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/io.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/export.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/mv_usb.h> +#include <linux/usb/phy.h> +#include <linux/usb/mv_usb2.h> + +/* phy regs */ +#define UTMI_REVISION 0x0 +#define UTMI_CTRL 0x4 +#define UTMI_PLL 0x8 +#define UTMI_TX 0xc +#define UTMI_RX 0x10 +#define UTMI_IVREF 0x14 +#define UTMI_T0 0x18 +#define UTMI_T1 0x1c +#define UTMI_T2 0x20 +#define UTMI_T3 0x24 +#define UTMI_T4 0x28 +#define UTMI_T5 0x2c +#define UTMI_RESERVE 0x30 +#define UTMI_USB_INT 0x34 +#define UTMI_DBG_CTL 0x38 +#define UTMI_OTG_ADDON 0x3c + +/* For UTMICTRL Register */ +#define UTMI_CTRL_USB_CLK_EN (1 << 31) +/* pxa168 */ +#define UTMI_CTRL_SUSPEND_SET1 (1 << 30) +#define UTMI_CTRL_SUSPEND_SET2 (1 << 29) +#define UTMI_CTRL_RXBUF_PDWN (1 << 24) +#define UTMI_CTRL_TXBUF_PDWN (1 << 11) + +#define UTMI_CTRL_INPKT_DELAY_SHIFT 30 +#define UTMI_CTRL_INPKT_DELAY_SOF_SHIFT 28 +#define UTMI_CTRL_PU_REF_SHIFT 20 +#define UTMI_CTRL_ARC_PULLDN_SHIFT 12 +#define UTMI_CTRL_PLL_PWR_UP_SHIFT 1 +#define UTMI_CTRL_PWR_UP_SHIFT 0 + +/* For UTMI_PLL Register */ +#define UTMI_PLL_PLLCALI12_SHIFT 29 +#define UTMI_PLL_PLLCALI12_MASK (0x3 << 29) + +#define UTMI_PLL_PLLVDD18_SHIFT 27 +#define UTMI_PLL_PLLVDD18_MASK (0x3 << 27) + +#define UTMI_PLL_PLLVDD12_SHIFT 25 +#define UTMI_PLL_PLLVDD12_MASK (0x3 << 25) + +#define UTMI_PLL_CLK_BLK_EN_SHIFT 24 +#define CLK_BLK_EN (0x1 << 24) +#define PLL_READY (0x1 << 23) +#define KVCO_EXT (0x1 << 22) +#define VCOCAL_START (0x1 << 21) + +#define UTMI_PLL_KVCO_SHIFT 15 +#define UTMI_PLL_KVCO_MASK (0x7 << 15) + +#define UTMI_PLL_ICP_SHIFT 12 +#define UTMI_PLL_ICP_MASK (0x7 << 12) + +#define UTMI_PLL_FBDIV_SHIFT 4 +#define UTMI_PLL_FBDIV_MASK (0xFF << 4) + +#define UTMI_PLL_REFDIV_SHIFT 0 +#define UTMI_PLL_REFDIV_MASK (0xF << 0) + +/* For UTMI_TX Register */ +#define UTMI_TX_REG_EXT_FS_RCAL_SHIFT 27 +#define UTMI_TX_REG_EXT_FS_RCAL_MASK (0xf << 27) + +#define UTMI_TX_REG_EXT_FS_RCAL_EN_SHIFT 26 +#define UTMI_TX_REG_EXT_FS_RCAL_EN_MASK (0x1 << 26) + +#define UTMI_TX_TXVDD12_SHIFT 22 +#define UTMI_TX_TXVDD12_MASK (0x3 << 22) + +#define UTMI_TX_CK60_PHSEL_SHIFT 17 +#define UTMI_TX_CK60_PHSEL_MASK (0xf << 17) + +#define UTMI_TX_IMPCAL_VTH_SHIFT 14 +#define UTMI_TX_IMPCAL_VTH_MASK (0x7 << 14) + +#define REG_RCAL_START (0x1 << 12) + +#define UTMI_TX_LOW_VDD_EN_SHIFT 11 + +#define UTMI_TX_AMP_SHIFT 0 +#define UTMI_TX_AMP_MASK (0x7 << 0) + +/* For UTMI_RX Register */ +#define UTMI_REG_SQ_LENGTH_SHIFT 15 +#define UTMI_REG_SQ_LENGTH_MASK (0x3 << 15) + +#define UTMI_RX_SQ_THRESH_SHIFT 4 +#define UTMI_RX_SQ_THRESH_MASK (0xf << 4) + +#define UTMI_OTG_ADDON_OTG_ON (1 << 0) + +enum mv_usb2_phy_type { + PXA168_USB, + PXA910_USB, + MMP2_USB, +}; + +static unsigned int u2o_get(void __iomem *base, unsigned int offset) +{ + return readl(base + offset); +} + +static void u2o_set(void __iomem *base, unsigned int offset, + unsigned int value) +{ + u32 reg; + + reg = readl(base + offset); + reg |= value; + writel(reg, base + offset); + /* + * read after write. It will make sure writing takes effect. + * It is suggested by PHY design engineer. + */ + readl(base + offset); +} + +static void u2o_clear(void __iomem *base, unsigned int offset, + unsigned int value) +{ + u32 reg; + + reg = readl(base + offset); + reg &= ~value; + writel(reg, base + offset); + /* + * read after write. It will make sure writing takes effect. + * It is suggested by PHY design engineer. + */ + readl(base + offset); +} + +static void u2o_write(void __iomem *base, unsigned int offset, + unsigned int value) +{ + writel(value, base + offset); + /* + * read after write. It will make sure writing takes effect. + * It is suggested by PHY design engineer. + */ + readl(base + offset); +} + +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy) +{ + struct platform_device *pdev = mv_phy->pdev; + unsigned int loops = 0; + void __iomem *base = mv_phy->base; + + dev_dbg(&pdev->dev, "phy init\n"); + + /* Initialize the USB PHY power */ + if (mv_phy->type == PXA910_USB) { + u2o_set(base, UTMI_CTRL, (1<<UTMI_CTRL_INPKT_DELAY_SOF_SHIFT) + | (1<<UTMI_CTRL_PU_REF_SHIFT)); + } + + u2o_set(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); + u2o_set(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); + + /* UTMI_PLL settings */ + u2o_clear(base, UTMI_PLL, UTMI_PLL_PLLVDD18_MASK + | UTMI_PLL_PLLVDD12_MASK | UTMI_PLL_PLLCALI12_MASK + | UTMI_PLL_FBDIV_MASK | UTMI_PLL_REFDIV_MASK + | UTMI_PLL_ICP_MASK | UTMI_PLL_KVCO_MASK); + + u2o_set(base, UTMI_PLL, 0xee<<UTMI_PLL_FBDIV_SHIFT + | 0xb<<UTMI_PLL_REFDIV_SHIFT | 3<<UTMI_PLL_PLLVDD18_SHIFT + | 3<<UTMI_PLL_PLLVDD12_SHIFT | 3<<UTMI_PLL_PLLCALI12_SHIFT + | 1<<UTMI_PLL_ICP_SHIFT | 3<<UTMI_PLL_KVCO_SHIFT); + + /* UTMI_TX */ + u2o_clear(base, UTMI_TX, UTMI_TX_REG_EXT_FS_RCAL_EN_MASK + | UTMI_TX_TXVDD12_MASK | UTMI_TX_CK60_PHSEL_MASK + | UTMI_TX_IMPCAL_VTH_MASK | UTMI_TX_REG_EXT_FS_RCAL_MASK + | UTMI_TX_AMP_MASK); + u2o_set(base, UTMI_TX, 3<<UTMI_TX_TXVDD12_SHIFT + | 4<<UTMI_TX_CK60_PHSEL_SHIFT | 4<<UTMI_TX_IMPCAL_VTH_SHIFT + | 8<<UTMI_TX_REG_EXT_FS_RCAL_SHIFT | 3<<UTMI_TX_AMP_SHIFT); + + /* UTMI_RX */ + u2o_clear(base, UTMI_RX, UTMI_RX_SQ_THRESH_MASK + | UTMI_REG_SQ_LENGTH_MASK); + u2o_set(base, UTMI_RX, 7<<UTMI_RX_SQ_THRESH_SHIFT + | 2<<UTMI_REG_SQ_LENGTH_SHIFT); + + /* UTMI_IVREF */ + if (mv_phy->type == PXA168_USB) + /* fixing Microsoft Altair board interface with NEC hub issue - + * Set UTMI_IVREF from 0x4a3 to 0x4bf */ + u2o_write(base, UTMI_IVREF, 0x4bf); + + /* toggle VCOCAL_START bit of UTMI_PLL */ + udelay(200); + u2o_set(base, UTMI_PLL, VCOCAL_START); + udelay(40); + u2o_clear(base, UTMI_PLL, VCOCAL_START); + + /* toggle REG_RCAL_START bit of UTMI_TX */ + udelay(400); + u2o_set(base, UTMI_TX, REG_RCAL_START); + udelay(40); + u2o_clear(base, UTMI_TX, REG_RCAL_START); + udelay(400); + + /* Make sure PHY PLL is ready */ + loops = 0; + while ((u2o_get(base, UTMI_PLL) & PLL_READY) == 0) { + mdelay(1); + loops++; + if (loops > 100) { + dev_warn(&pdev->dev, "calibrate timeout, UTMI_PLL %x\n", + u2o_get(base, UTMI_PLL)); + break; + } + } + + if (mv_phy->type == PXA168_USB) { + u2o_set(base, UTMI_RESERVE, 1 << 5); + /* Turn on UTMI PHY OTG extension */ + u2o_write(base, UTMI_OTG_ADDON, 1); + } + + return 0; +} + +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy) +{ + void __iomem *base = mv_phy->base; + + if (mv_phy->type == PXA168_USB) + u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON); + + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN); + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN); + u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN); + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT); + u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT); + + return 0; +} + +static int mv_usb2_phy_init(struct usb_phy *phy) +{ + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); + int i = 0; + + mutex_lock(&mv_phy->phy_lock); + if (mv_phy->refcount++ == 0) { + for (i = 0; i < mv_phy->clks_num; i++) + clk_prepare_enable(mv_phy->clks[i]); + _mv_usb2_phy_init(mv_phy); + } + mutex_unlock(&mv_phy->phy_lock); + return 0; +} + +static void mv_usb2_phy_shutdown(struct usb_phy *phy) +{ + struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy); + int i = 0; + + mutex_lock(&mv_phy->phy_lock); + if (--mv_phy->refcount == 0) { + _mv_usb2_phy_shutdown(mv_phy); + for (i = 0; i < mv_phy->clks_num; i++) + clk_disable_unprepare(mv_phy->clks[i]); + } + mutex_unlock(&mv_phy->phy_lock); +} + +static int mv_usb2_phy_probe(struct platform_device *pdev) +{ + struct mv_usb2_phy *mv_phy; + struct resource *r; + int i; + struct mv_usb_phy_platform_data *pdata; + const struct platform_device_id *id; + + mv_phy = devm_kzalloc(&pdev->dev, sizeof(*mv_phy), GFP_KERNEL); + if (mv_phy == NULL) { + dev_err(&pdev->dev, "failed to allocate memory\n"); + return -ENOMEM; + } + mutex_init(&mv_phy->phy_lock); + + pdata = pdev->dev.platform_data; + id = platform_get_device_id(pdev); + + if (pdata == NULL || id == NULL) { + dev_err(&pdev->dev, + "missing platform_data or id_entry\n"); + return -ENODEV; + } + mv_phy->type = (unsigned int)(id->driver_data); + mv_phy->clks_num = pdata->clknum; + mv_phy->clks = devm_kzalloc(&pdev->dev, + sizeof(struct clk *) * mv_phy->clks_num, GFP_KERNEL); + if (mv_phy->clks == NULL) { + dev_err(&pdev->dev, + "failed to allocate mempory for clocks"); + return -ENOMEM; + } + for (i = 0; i < mv_phy->clks_num; i++) { + mv_phy->clks[i] = devm_clk_get(&pdev->dev, + pdata->clkname[i]); + if (IS_ERR(mv_phy->clks[i])) { + dev_err(&pdev->dev, "failed to get clock %s\n", + pdata->clkname[i]); + return PTR_ERR(mv_phy->clks[i]); + } + } + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (r == NULL) { + dev_err(&pdev->dev, "no phy I/O memory resource defined\n"); + return -ENODEV; + } + mv_phy->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); + if (mv_phy->base == NULL) { + dev_err(&pdev->dev, "error map register base\n"); + return -EBUSY; + } + + mv_phy->phy.dev = &pdev->dev; + mv_phy->phy.label = "mv-usb2"; + mv_phy->phy.type = USB_PHY_TYPE_USB2; + mv_phy->phy.init = mv_usb2_phy_init; + mv_phy->phy.shutdown = mv_usb2_phy_shutdown; + + usb_add_phy_dev(&mv_phy->phy); + + platform_set_drvdata(pdev, mv_phy); + + return 0; +} + +static int mv_usb2_phy_remove(struct platform_device *pdev) +{ + struct mv_usb2_phy *mv_phy = platform_get_drvdata(pdev); + + usb_remove_phy(&mv_phy->phy); + + platform_set_drvdata(pdev, NULL); + + return 0; +} + +static struct platform_device_id mv_usb2_phy_ids[] = { + { .name = "pxa168-usb-phy", .driver_data = PXA168_USB }, + { .name = "pxa910-usb-phy", .driver_data = PXA910_USB }, + { .name = "mmp2-usb-phy", .driver_data = MMP2_USB }, + {} +}; + +static struct platform_driver mv_usb2_phy_driver = { + .probe = mv_usb2_phy_probe, + .remove = mv_usb2_phy_remove, + .driver = { + .name = "pxa168-usb-phy", + }, + .id_table = mv_usb2_phy_ids, +}; + +static int __init mv_usb2_phy_driver_init(void) +{ + return platform_driver_register(&mv_usb2_phy_driver); +} +arch_initcall(mv_usb2_phy_driver_init); diff --git a/include/linux/platform_data/mv_usb.h b/include/linux/platform_data/mv_usb.h index 944b01d..fd3d1b4 100644 --- a/include/linux/platform_data/mv_usb.h +++ b/include/linux/platform_data/mv_usb.h @@ -47,9 +47,12 @@ struct mv_usb_platform_data { /* Force a_bus_req to be asserted */ unsigned int otg_force_a_bus_req:1; - int (*phy_init)(void __iomem *regbase); - void (*phy_deinit)(void __iomem *regbase); int (*set_vbus)(unsigned int vbus); - int (*private_init)(void __iomem *opregs, void __iomem *phyregs); }; + +struct mv_usb_phy_platform_data { + unsigned int clknum; + char **clkname; +}; + #endif diff --git a/include/linux/usb/mv_usb2.h b/include/linux/usb/mv_usb2.h new file mode 100644 index 0000000..77250f5 --- /dev/null +++ b/include/linux/usb/mv_usb2.h @@ -0,0 +1,32 @@ +/* + * Copyright (C) 2013 Marvell Inc. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef __MV_USB2_H +#define __MV_USB2_H + +#define MV_USB2_PHY_INDEX 0 +#define MV_USB2_OTG_PHY_INDEX 1 + +struct mv_usb2_phy { + struct usb_phy phy; + struct platform_device *pdev; + struct mutex phy_lock; + unsigned int refcount; + unsigned int type; + void __iomem *base; + struct clk **clks; + unsigned int clks_num; +}; + +#endif
The PHY is seperated from usb controller. The usb controller used in marvell pxa168/pxa910/mmp2 are same, but PHY initialization may be different. the usb controller can support udc/otg/ehci, and for each of the mode, it need PHY to initialized before use the controller. Direclty writing the phy driver will make the usb controller driver to be simple and portable. The PHY driver will be used by marvell udc/otg/ehci. Signed-off-by: Chao Xie <chao.xie@marvell.com> --- drivers/usb/phy/Kconfig | 7 + drivers/usb/phy/Makefile | 1 + drivers/usb/phy/mv_usb2_phy.c | 402 ++++++++++++++++++++++++++++++++++ include/linux/platform_data/mv_usb.h | 9 +- include/linux/usb/mv_usb2.h | 32 +++ 5 files changed, 448 insertions(+), 3 deletions(-) create mode 100644 drivers/usb/phy/mv_usb2_phy.c create mode 100644 include/linux/usb/mv_usb2.h