| Message ID | 1342076512-19207-13-git-send-email-richard.zhao@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[snip] > --- /dev/null > +++ b/drivers/usb/chipidea/imx_usbmisc.c > @@ -0,0 +1,144 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <asm/io.h> It should be linux/io.h Thanks Richard
On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: > i.MX usb controllers shares non-core registers, which may include > SoC specific controls. We take it as a usbmisc device and usbmisc > driver export functions needed by ci13xxx_imx driver. So this is SoC and not chipidea specific and should not be sorted into this subdir. > For example, Sabrelite board has bad over-current design, we can > usbmisc to disable over-current detect. This driver layout is also reflected in: arch/arm/mach-imx/ehci-imx*.c You should use these existing mx*_initialize_usb_hw functions to avoid code duplication and add your mx6_initialize_usb_hw routine for mx6 there. This devicetree based glue code in this file can be reused to call the right mx*_initialize_usb_hw by the compatible. We already have common flags available like i.e. MXC_EHCI_POWER_PINS_ENABLED which is currently used to disable overcurrent detection. > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > --- > .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ > drivers/usb/chipidea/Makefile | 2 +- > drivers/usb/chipidea/ci13xxx_imx.c | 4 + > drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ > 4 files changed, 164 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt > create mode 100644 drivers/usb/chipidea/imx_usbmisc.c > > diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > new file mode 100644 > index 0000000..09f01ca > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > @@ -0,0 +1,15 @@ > +* Freescale i.MX non-core registers > + > +Required properties: > +- compatible: Should be "fsl,imx6q-usbmisc" > +- reg: Should contain registers location and length > + > +Optional properties: > +- fsl,disable-over-current: disable over current detect > + > +Examples: > +usbmisc@02184800 { > + compatible = "fsl,imx6q-usbmisc"; > + reg = <0x02184800 0x200>; > + fsl,disable-over-current; > +}; > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 3f56b76..46fc31c 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) > endif > > ifneq ($(CONFIG_OF_DEVICE),) > - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o > + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o > endif > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > index b3173d8..dd7f3a3 100644 > --- a/drivers/usb/chipidea/ci13xxx_imx.c > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > @@ -24,6 +24,8 @@ > > #include "ci.h" > > +int imx_usbmisc_init(struct device *usbdev); > + > #define pdev_to_phy(pdev) \ > ((struct usb_phy *)platform_get_drvdata(pdev)) > #define ci_to_imx_data(ci) \ > @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); > } > > + imx_usbmisc_init(&pdev->dev); > + > platform_set_drvdata(pdev, data); > > plat_ci = ci13xxx_add_device(&pdev->dev, > diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c > new file mode 100644 > index 0000000..33a8ec0 > --- /dev/null > +++ b/drivers/usb/chipidea/imx_usbmisc.c > @@ -0,0 +1,144 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <asm/io.h> > + > +struct usbmisc; > + > +struct soc_data { > + int (*init) (struct usbmisc *usbmisc, int id); > + void (*exit) (struct usbmisc *usbmisc, int id); > +}; > + > +struct usbmisc { > + struct soc_data *soc_data; > + void __iomem *base; > + struct clk *clk; > + > + int dis_oc:1; > +}; > + > +/* Since we only have one usbmisc device at runtime, we global variables */ > +static struct usbmisc *usbmisc; Global variable? > + > +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) What is initialized here? How about "preconfigure"? Even better is to reuse mx*_initialize_usb_hw here. > +{ > + u32 reg; > + > + if (usbmisc->dis_oc) { > + reg = readl_relaxed(usbmisc->base + id * 4); > + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); #define IMX6_OTG_CTRL_USBMISC 1 << 7 > + } > + > + return 0; > +} > + > +static struct soc_data imx6q_data = { > + .init = imx6q_usbmisc_init, > +}; > + > + > +static const struct of_device_id imx_usbmisc_dt_ids[] = { > + { .compatible = "fsl,imx6q-usbmisc", .data = &imx6q_data }, something like this: { .compatible = "fsl,imx6q-usbmisc", .data = &mx6_initialize_usb_hw }, > + { /* sentinel */ } > +}; > + > +static int __devinit imx_usbmisc_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct usbmisc *data; > + const struct of_device_id *of_id = > + of_match_device(imx_usbmisc_dt_ids, &pdev->dev); > + > + int ret; > + > + if (usbmisc) > + return -EBUSY; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_request_and_ioremap(&pdev->dev, res); > + if (!data->base) > + return -EADDRNOTAVAIL; > + > + data->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(data->clk)) { > + dev_err(&pdev->dev, > + "failed to get clock, err=%ld\n", PTR_ERR(data->clk)); > + return PTR_ERR(data->clk); > + } > + > + ret = clk_prepare_enable(data->clk); > + if (!ret) { Better: if (ret) return ret; > + if (of_find_property(pdev->dev.of_node, > + "fsl,disable-over-current", NULL)) > + data->dis_oc = 1; > + data->soc_data = of_id->data; > + usbmisc = data; > + } > + if (of_find_property(pdev->dev.of_node, ... > + return ret; > +} > + > +static int __devexit imx_usbmisc_remove(struct platform_device *pdev) > +{ > + clk_disable_unprepare(usbmisc->clk); > + return 0; > +} > + > +static struct platform_driver imx_usbmisc_driver = { > + .probe = imx_usbmisc_probe, > + .remove = __devexit_p(imx_usbmisc_remove), > + .driver = { > + .name = "imx_usbmisc", > + .owner = THIS_MODULE, > + .of_match_table = imx_usbmisc_dt_ids, > + }, > +}; > + > +int imx_usbmisc_init(struct device *usbdev) > +{ > + struct device_node *np = usbdev->of_node; > + int ret; > + > + if (!usbmisc) > + return 0; > + > + ret = of_alias_get_id(np, "usb"); > + if (ret < 0) { > + dev_err(usbdev, "failed to get alias id, errno %d\n", ret); > + return ret; > + } > + > + return usbmisc->soc_data->init(usbmisc, ret); > +} > +EXPORT_SYMBOL_GPL(imx_usbmisc_init); EXPORT_SYMBOL is only needed because it is compliled as an extra kernel module. > + > +static int __init imx_usbmisc_drv_init(void) > +{ > + return platform_driver_register(&imx_usbmisc_driver); > +} > +subsys_initcall(imx_usbmisc_drv_init); > + > +static void __exit imx_usbmisc_drv_exit(void) > +{ > + platform_driver_unregister(&imx_usbmisc_driver); > +} > +module_exit(imx_usbmisc_drv_exit); > + > +MODULE_LICENSE("GPL v2"); > -- > 1.7.9.5 Thanks, Michael
On Fri, Jul 13, 2012 at 02:25:45PM +0200, Michael Grzeschik wrote: > On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: > > i.MX usb controllers shares non-core registers, which may include > > SoC specific controls. We take it as a usbmisc device and usbmisc > > driver export functions needed by ci13xxx_imx driver. > > So this is SoC and not chipidea specific and should not be sorted into > this subdir. Yes, but it's usb specific and serve chipidea usb IP. > > > For example, Sabrelite board has bad over-current design, we can > > usbmisc to disable over-current detect. > > This driver layout is also reflected in: > > arch/arm/mach-imx/ehci-imx*.c It sounds sensible, but I'm not quite sure. I saw drivers are moving out of there, event it's SoC specific, for example, clk, pinmux. And the non-core registers might not only be setup onetime on boot, but be called at runtime by usb driver. For now, it's set once. > > You should use these existing mx*_initialize_usb_hw functions to avoid > code duplication I can not see what duplication it can avoid. > and add your mx6_initialize_usb_hw routine for mx6 there. Maybe. > > This devicetree based glue code in this file can be reused to > call the right mx*_initialize_usb_hw by the compatible. The glue code makes it more like a normal driver. Doesn't it? > > We already have common flags available like i.e. MXC_EHCI_POWER_PINS_ENABLED > which is currently used to disable overcurrent detection. The flags may be replace with properties in DT bindings. > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > > --- > > .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ > > drivers/usb/chipidea/Makefile | 2 +- > > drivers/usb/chipidea/ci13xxx_imx.c | 4 + > > drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ > > 4 files changed, 164 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > create mode 100644 drivers/usb/chipidea/imx_usbmisc.c > > > > diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > new file mode 100644 > > index 0000000..09f01ca > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > @@ -0,0 +1,15 @@ > > +* Freescale i.MX non-core registers > > + > > +Required properties: > > +- compatible: Should be "fsl,imx6q-usbmisc" > > +- reg: Should contain registers location and length > > + > > +Optional properties: > > +- fsl,disable-over-current: disable over current detect > > + > > +Examples: > > +usbmisc@02184800 { > > + compatible = "fsl,imx6q-usbmisc"; > > + reg = <0x02184800 0x200>; > > + fsl,disable-over-current; > > +}; > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > > index 3f56b76..46fc31c 100644 > > --- a/drivers/usb/chipidea/Makefile > > +++ b/drivers/usb/chipidea/Makefile > > @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) > > endif > > > > ifneq ($(CONFIG_OF_DEVICE),) > > - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o > > + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o > > endif > > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > > index b3173d8..dd7f3a3 100644 > > --- a/drivers/usb/chipidea/ci13xxx_imx.c > > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > > @@ -24,6 +24,8 @@ > > > > #include "ci.h" > > > > +int imx_usbmisc_init(struct device *usbdev); > > + > > #define pdev_to_phy(pdev) \ > > ((struct usb_phy *)platform_get_drvdata(pdev)) > > #define ci_to_imx_data(ci) \ > > @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > > dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); > > } > > > > + imx_usbmisc_init(&pdev->dev); > > + > > platform_set_drvdata(pdev, data); > > > > plat_ci = ci13xxx_add_device(&pdev->dev, > > diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c > > new file mode 100644 > > index 0000000..33a8ec0 > > --- /dev/null > > +++ b/drivers/usb/chipidea/imx_usbmisc.c > > @@ -0,0 +1,144 @@ > > +/* > > + * Copyright 2012 Freescale Semiconductor, Inc. > > + * > > + * The code contained herein is licensed under the GNU General Public > > + * License. You may obtain a copy of the GNU General Public License > > + * Version 2 or later at the following locations: > > + * > > + * http://www.opensource.org/licenses/gpl-license.html > > + * http://www.gnu.org/copyleft/gpl.html > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/of_platform.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <asm/io.h> > > + > > +struct usbmisc; > > + > > +struct soc_data { > > + int (*init) (struct usbmisc *usbmisc, int id); > > + void (*exit) (struct usbmisc *usbmisc, int id); > > +}; > > + > > +struct usbmisc { > > + struct soc_data *soc_data; > > + void __iomem *base; > > + struct clk *clk; > > + > > + int dis_oc:1; > > +}; > > + > > +/* Since we only have one usbmisc device at runtime, we global variables */ > > +static struct usbmisc *usbmisc; > > Global variable? Yes. we only have one usbmisc. > > > + > > +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) > > What is initialized here? How about "preconfigure"? > Even better is to reuse mx*_initialize_usb_hw here. It's used to call by ci13xxx_imx driver. > > > +{ > > + u32 reg; > > + > > + if (usbmisc->dis_oc) { > > + reg = readl_relaxed(usbmisc->base + id * 4); > > + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); > > #define IMX6_OTG_CTRL_USBMISC 1 << 7 yes. > > > + } > > + > > + return 0; > > +} > > + > > +static struct soc_data imx6q_data = { > > + .init = imx6q_usbmisc_init, > > +}; > > + > > + > > +static const struct of_device_id imx_usbmisc_dt_ids[] = { > > + { .compatible = "fsl,imx6q-usbmisc", .data = &imx6q_data }, > > something like this: > { .compatible = "fsl,imx6q-usbmisc", .data = &mx6_initialize_usb_hw }, It's designed not only used when initialization. > > > + { /* sentinel */ } > > +}; > > + > > +static int __devinit imx_usbmisc_probe(struct platform_device *pdev) > > +{ > > + struct resource *res; > > + struct usbmisc *data; > > + const struct of_device_id *of_id = > > + of_match_device(imx_usbmisc_dt_ids, &pdev->dev); > > + > > + int ret; > > + > > + if (usbmisc) > > + return -EBUSY; > > + > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + data->base = devm_request_and_ioremap(&pdev->dev, res); > > + if (!data->base) > > + return -EADDRNOTAVAIL; > > + > > + data->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(data->clk)) { > > + dev_err(&pdev->dev, > > + "failed to get clock, err=%ld\n", PTR_ERR(data->clk)); > > + return PTR_ERR(data->clk); > > + } > > + > > + ret = clk_prepare_enable(data->clk); > > + if (!ret) { > > Better: > if (ret) > return ret; To me, it does not make much difference. > > > + if (of_find_property(pdev->dev.of_node, > > + "fsl,disable-over-current", NULL)) > > + data->dis_oc = 1; > > + data->soc_data = of_id->data; > > + usbmisc = data; > > + } > > + > if (of_find_property(pdev->dev.of_node, > ... > > > + return ret; > > +} > > + > > +static int __devexit imx_usbmisc_remove(struct platform_device *pdev) > > +{ > > + clk_disable_unprepare(usbmisc->clk); > > + return 0; > > +} > > + > > +static struct platform_driver imx_usbmisc_driver = { > > + .probe = imx_usbmisc_probe, > > + .remove = __devexit_p(imx_usbmisc_remove), > > + .driver = { > > + .name = "imx_usbmisc", > > + .owner = THIS_MODULE, > > + .of_match_table = imx_usbmisc_dt_ids, > > + }, > > +}; > > + > > +int imx_usbmisc_init(struct device *usbdev) > > +{ > > + struct device_node *np = usbdev->of_node; > > + int ret; > > + > > + if (!usbmisc) > > + return 0; > > + > > + ret = of_alias_get_id(np, "usb"); > > + if (ret < 0) { > > + dev_err(usbdev, "failed to get alias id, errno %d\n", ret); > > + return ret; > > + } > > + > > + return usbmisc->soc_data->init(usbmisc, ret); > > +} > > +EXPORT_SYMBOL_GPL(imx_usbmisc_init); > > EXPORT_SYMBOL is only needed because it is > compliled as an extra kernel module. ci13xxx_imx may be module. Thanks Richard > > > + > > +static int __init imx_usbmisc_drv_init(void) > > +{ > > + return platform_driver_register(&imx_usbmisc_driver); > > +} > > +subsys_initcall(imx_usbmisc_drv_init); > > + > > +static void __exit imx_usbmisc_drv_exit(void) > > +{ > > + platform_driver_unregister(&imx_usbmisc_driver); > > +} > > +module_exit(imx_usbmisc_drv_exit); > > + > > +MODULE_LICENSE("GPL v2"); > > -- > > 1.7.9.5 > > Thanks, > Michael > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2012 04:02 PM, Richard Zhao wrote: > On Fri, Jul 13, 2012 at 02:25:45PM +0200, Michael Grzeschik wrote: >> On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: >>> i.MX usb controllers shares non-core registers, which may include >>> SoC specific controls. We take it as a usbmisc device and usbmisc >>> driver export functions needed by ci13xxx_imx driver. >> >> So this is SoC and not chipidea specific and should not be sorted into >> this subdir. > Yes, but it's usb specific and serve chipidea usb IP. I don't care where this code is located. >>> For example, Sabrelite board has bad over-current design, we can >>> usbmisc to disable over-current detect. >> >> This driver layout is also reflected in: >> >> arch/arm/mach-imx/ehci-imx*.c > It sounds sensible, but I'm not quite sure. I saw drivers are moving > out of there, event it's SoC specific, for example, clk, pinmux. And > the non-core registers might not only be setup onetime on boot, but > be called at runtime by usb driver. For now, it's set once. >> >> You should use these existing mx*_initialize_usb_hw functions to avoid >> code duplication > I can not see what duplication it can avoid. We want to use the chipidea driver for the other imx, too. We already have existing code for the non-DT case in arch/arm/mach-imx/ehci-imx*.c. So we need to glue these functions to the DT or duplicate the code, which is to be avoided. >> and add your mx6_initialize_usb_hw routine for mx6 there. > Maybe. Or at least next to the other imx usb-misc code (wherever that will end up). >> This devicetree based glue code in this file can be reused to >> call the right mx*_initialize_usb_hw by the compatible. > The glue code makes it more like a normal driver. Doesn't it? >> >> We already have common flags available like i.e. MXC_EHCI_POWER_PINS_ENABLED >> which is currently used to disable overcurrent detection. > The flags may be replace with properties in DT bindings. Yes, write some code that extracts these flags from the DT and then calls the existing code (for the existing imx platforms). >>> Signed-off-by: Richard Zhao <richard.zhao@freescale.com> >>> --- >>> .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ >>> drivers/usb/chipidea/Makefile | 2 +- >>> drivers/usb/chipidea/ci13xxx_imx.c | 4 + >>> drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ >>> 4 files changed, 164 insertions(+), 1 deletion(-) >>> create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt >>> create mode 100644 drivers/usb/chipidea/imx_usbmisc.c >>> >>> diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt >>> new file mode 100644 >>> index 0000000..09f01ca >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt >>> @@ -0,0 +1,15 @@ >>> +* Freescale i.MX non-core registers >>> + >>> +Required properties: >>> +- compatible: Should be "fsl,imx6q-usbmisc" >>> +- reg: Should contain registers location and length >>> + >>> +Optional properties: >>> +- fsl,disable-over-current: disable over current detect >>> + >>> +Examples: >>> +usbmisc@02184800 { >>> + compatible = "fsl,imx6q-usbmisc"; >>> + reg = <0x02184800 0x200>; >>> + fsl,disable-over-current; >>> +}; >>> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile >>> index 3f56b76..46fc31c 100644 >>> --- a/drivers/usb/chipidea/Makefile >>> +++ b/drivers/usb/chipidea/Makefile >>> @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) >>> endif >>> >>> ifneq ($(CONFIG_OF_DEVICE),) >>> - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o >>> + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o >>> endif >>> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c >>> index b3173d8..dd7f3a3 100644 >>> --- a/drivers/usb/chipidea/ci13xxx_imx.c >>> +++ b/drivers/usb/chipidea/ci13xxx_imx.c >>> @@ -24,6 +24,8 @@ >>> >>> #include "ci.h" >>> >>> +int imx_usbmisc_init(struct device *usbdev); >>> + >>> #define pdev_to_phy(pdev) \ >>> ((struct usb_phy *)platform_get_drvdata(pdev)) >>> #define ci_to_imx_data(ci) \ >>> @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) >>> dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); >>> } >>> >>> + imx_usbmisc_init(&pdev->dev); >>> + >>> platform_set_drvdata(pdev, data); >>> >>> plat_ci = ci13xxx_add_device(&pdev->dev, >>> diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c >>> new file mode 100644 >>> index 0000000..33a8ec0 >>> --- /dev/null >>> +++ b/drivers/usb/chipidea/imx_usbmisc.c >>> @@ -0,0 +1,144 @@ >>> +/* >>> + * Copyright 2012 Freescale Semiconductor, Inc. >>> + * >>> + * The code contained herein is licensed under the GNU General Public >>> + * License. You may obtain a copy of the GNU General Public License >>> + * Version 2 or later at the following locations: >>> + * >>> + * http://www.opensource.org/licenses/gpl-license.html >>> + * http://www.gnu.org/copyleft/gpl.html >>> + */ >>> + >>> +#include <linux/module.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/clk.h> >>> +#include <linux/err.h> >>> +#include <asm/io.h> >>> + >>> +struct usbmisc; >>> + >>> +struct soc_data { >>> + int (*init) (struct usbmisc *usbmisc, int id); >>> + void (*exit) (struct usbmisc *usbmisc, int id); >>> +}; >>> + >>> +struct usbmisc { >>> + struct soc_data *soc_data; >>> + void __iomem *base; >>> + struct clk *clk; >>> + >>> + int dis_oc:1; >>> +}; >>> + >>> +/* Since we only have one usbmisc device at runtime, we global variables */ >>> +static struct usbmisc *usbmisc; >> >> Global variable? > Yes. we only have one usbmisc. >> >>> + >>> +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) >> >> What is initialized here? How about "preconfigure"? >> Even better is to reuse mx*_initialize_usb_hw here. > It's used to call by ci13xxx_imx driver. >> >>> +{ >>> + u32 reg; >>> + >>> + if (usbmisc->dis_oc) { >>> + reg = readl_relaxed(usbmisc->base + id * 4); >>> + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); >> >> #define IMX6_OTG_CTRL_USBMISC 1 << 7 > yes. >> >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct soc_data imx6q_data = { >>> + .init = imx6q_usbmisc_init, >>> +}; >>> + >>> + >>> +static const struct of_device_id imx_usbmisc_dt_ids[] = { >>> + { .compatible = "fsl,imx6q-usbmisc", .data = &imx6q_data }, >> >> something like this: >> { .compatible = "fsl,imx6q-usbmisc", .data = &mx6_initialize_usb_hw }, > It's designed not only used when initialization. >> >>> + { /* sentinel */ } >>> +}; >>> + >>> +static int __devinit imx_usbmisc_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res; >>> + struct usbmisc *data; >>> + const struct of_device_id *of_id = >>> + of_match_device(imx_usbmisc_dt_ids, &pdev->dev); >>> + >>> + int ret; >>> + >>> + if (usbmisc) >>> + return -EBUSY; >>> + >>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >>> + if (!data) >>> + return -ENOMEM; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + data->base = devm_request_and_ioremap(&pdev->dev, res); >>> + if (!data->base) >>> + return -EADDRNOTAVAIL; >>> + >>> + data->clk = devm_clk_get(&pdev->dev, NULL); >>> + if (IS_ERR(data->clk)) { >>> + dev_err(&pdev->dev, >>> + "failed to get clock, err=%ld\n", PTR_ERR(data->clk)); >>> + return PTR_ERR(data->clk); >>> + } >>> + >>> + ret = clk_prepare_enable(data->clk); >>> + if (!ret) { >> >> Better: >> if (ret) >> return ret; > To me, it does not make much difference. Not the the compiler, but IMHO the common pattern in the kernel is: ret = sensible_init(); if (ret) return ret; do_work(); >> >>> + if (of_find_property(pdev->dev.of_node, >>> + "fsl,disable-over-current", NULL)) >>> + data->dis_oc = 1; >>> + data->soc_data = of_id->data; >>> + usbmisc = data; >>> + } >>> + >> if (of_find_property(pdev->dev.of_node, >> ... >> >>> + return ret; >>> +} >>> + >>> +static int __devexit imx_usbmisc_remove(struct platform_device *pdev) >>> +{ >>> + clk_disable_unprepare(usbmisc->clk); >>> + return 0; >>> +} >>> + >>> +static struct platform_driver imx_usbmisc_driver = { >>> + .probe = imx_usbmisc_probe, >>> + .remove = __devexit_p(imx_usbmisc_remove), >>> + .driver = { >>> + .name = "imx_usbmisc", >>> + .owner = THIS_MODULE, >>> + .of_match_table = imx_usbmisc_dt_ids, >>> + }, >>> +}; >>> + >>> +int imx_usbmisc_init(struct device *usbdev) >>> +{ >>> + struct device_node *np = usbdev->of_node; >>> + int ret; >>> + >>> + if (!usbmisc) >>> + return 0; >>> + >>> + ret = of_alias_get_id(np, "usb"); >>> + if (ret < 0) { >>> + dev_err(usbdev, "failed to get alias id, errno %d\n", ret); >>> + return ret; >>> + } >>> + >>> + return usbmisc->soc_data->init(usbmisc, ret); >>> +} >>> +EXPORT_SYMBOL_GPL(imx_usbmisc_init); >> >> EXPORT_SYMBOL is only needed because it is >> compliled as an extra kernel module. > ci13xxx_imx may be module. Why make the usb_misc an independent module, why not just link it into the ci13xxx_imx.ko Marc
Sascha & Shawn, Felipe & Alex, What's your opinion where I put this driver? The usbmisc drivers is SoC specific and provide callbacks to ci13xxx_imx driver at runtime. - How to organize the driver This patch is designed to put all per-soc driver in one file. But I plan to put it in discrete files. - Where to put it Option 1: arch/arm/mach-imx/ehci-imxXXX.c Option 2: drivers/usb/chipidea/imx/ , yes, we may need a folder. Both is fine for me. On Fri, Jul 13, 2012 at 04:14:27PM +0200, Marc Kleine-Budde wrote: > On 07/13/2012 04:02 PM, Richard Zhao wrote: > > On Fri, Jul 13, 2012 at 02:25:45PM +0200, Michael Grzeschik wrote: > >> On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: > >>> i.MX usb controllers shares non-core registers, which may include > >>> SoC specific controls. We take it as a usbmisc device and usbmisc > >>> driver export functions needed by ci13xxx_imx driver. > >> > >> So this is SoC and not chipidea specific and should not be sorted into > >> this subdir. > > Yes, but it's usb specific and serve chipidea usb IP. > > I don't care where this code is located. I need maintainers' instruction here. :) > > >>> For example, Sabrelite board has bad over-current design, we can > >>> usbmisc to disable over-current detect. > >> > >> This driver layout is also reflected in: > >> > >> arch/arm/mach-imx/ehci-imx*.c > > It sounds sensible, but I'm not quite sure. I saw drivers are moving > > out of there, event it's SoC specific, for example, clk, pinmux. And > > the non-core registers might not only be setup onetime on boot, but > > be called at runtime by usb driver. For now, it's set once. > >> > >> You should use these existing mx*_initialize_usb_hw functions to avoid > >> code duplication > > > I can not see what duplication it can avoid. > > We want to use the chipidea driver for the other imx, too. We already > have existing code for the non-DT case in arch/arm/mach-imx/ehci-imx*.c. > So we need to glue these functions to the DT or duplicate the code, > which is to be avoided. The existing code is too simple. ci13xxx_imx may call them at runtime. > > >> and add your mx6_initialize_usb_hw routine for mx6 there. > > Maybe. > > Or at least next to the other imx usb-misc code (wherever that will end up). > > >> This devicetree based glue code in this file can be reused to > >> call the right mx*_initialize_usb_hw by the compatible. > > The glue code makes it more like a normal driver. Doesn't it? > >> > >> We already have common flags available like i.e. MXC_EHCI_POWER_PINS_ENABLED > >> which is currently used to disable overcurrent detection. > > The flags may be replace with properties in DT bindings. > > Yes, write some code that extracts these flags from the DT and then > calls the existing code (for the existing imx platforms). > > >>> Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > >>> --- > >>> .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ > >>> drivers/usb/chipidea/Makefile | 2 +- > >>> drivers/usb/chipidea/ci13xxx_imx.c | 4 + > >>> drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ > >>> 4 files changed, 164 insertions(+), 1 deletion(-) > >>> create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt > >>> create mode 100644 drivers/usb/chipidea/imx_usbmisc.c > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > >>> new file mode 100644 > >>> index 0000000..09f01ca > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > >>> @@ -0,0 +1,15 @@ > >>> +* Freescale i.MX non-core registers > >>> + > >>> +Required properties: > >>> +- compatible: Should be "fsl,imx6q-usbmisc" > >>> +- reg: Should contain registers location and length > >>> + > >>> +Optional properties: > >>> +- fsl,disable-over-current: disable over current detect > >>> + > >>> +Examples: > >>> +usbmisc@02184800 { > >>> + compatible = "fsl,imx6q-usbmisc"; > >>> + reg = <0x02184800 0x200>; > >>> + fsl,disable-over-current; > >>> +}; > >>> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > >>> index 3f56b76..46fc31c 100644 > >>> --- a/drivers/usb/chipidea/Makefile > >>> +++ b/drivers/usb/chipidea/Makefile > >>> @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) > >>> endif > >>> > >>> ifneq ($(CONFIG_OF_DEVICE),) > >>> - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o > >>> + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o > >>> endif > >>> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > >>> index b3173d8..dd7f3a3 100644 > >>> --- a/drivers/usb/chipidea/ci13xxx_imx.c > >>> +++ b/drivers/usb/chipidea/ci13xxx_imx.c > >>> @@ -24,6 +24,8 @@ > >>> > >>> #include "ci.h" > >>> > >>> +int imx_usbmisc_init(struct device *usbdev); > >>> + > >>> #define pdev_to_phy(pdev) \ > >>> ((struct usb_phy *)platform_get_drvdata(pdev)) > >>> #define ci_to_imx_data(ci) \ > >>> @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > >>> dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); > >>> } > >>> > >>> + imx_usbmisc_init(&pdev->dev); > >>> + > >>> platform_set_drvdata(pdev, data); > >>> > >>> plat_ci = ci13xxx_add_device(&pdev->dev, > >>> diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c > >>> new file mode 100644 > >>> index 0000000..33a8ec0 > >>> --- /dev/null > >>> +++ b/drivers/usb/chipidea/imx_usbmisc.c > >>> @@ -0,0 +1,144 @@ > >>> +/* > >>> + * Copyright 2012 Freescale Semiconductor, Inc. > >>> + * > >>> + * The code contained herein is licensed under the GNU General Public > >>> + * License. You may obtain a copy of the GNU General Public License > >>> + * Version 2 or later at the following locations: > >>> + * > >>> + * http://www.opensource.org/licenses/gpl-license.html > >>> + * http://www.gnu.org/copyleft/gpl.html > >>> + */ > >>> + > >>> +#include <linux/module.h> > >>> +#include <linux/of_platform.h> > >>> +#include <linux/clk.h> > >>> +#include <linux/err.h> > >>> +#include <asm/io.h> > >>> + > >>> +struct usbmisc; > >>> + > >>> +struct soc_data { > >>> + int (*init) (struct usbmisc *usbmisc, int id); > >>> + void (*exit) (struct usbmisc *usbmisc, int id); > >>> +}; > >>> + > >>> +struct usbmisc { > >>> + struct soc_data *soc_data; > >>> + void __iomem *base; > >>> + struct clk *clk; > >>> + > >>> + int dis_oc:1; > >>> +}; > >>> + > >>> +/* Since we only have one usbmisc device at runtime, we global variables */ > >>> +static struct usbmisc *usbmisc; > >> > >> Global variable? > > Yes. we only have one usbmisc. > >> > >>> + > >>> +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) > >> > >> What is initialized here? How about "preconfigure"? > >> Even better is to reuse mx*_initialize_usb_hw here. > > It's used to call by ci13xxx_imx driver. > >> > >>> +{ > >>> + u32 reg; > >>> + > >>> + if (usbmisc->dis_oc) { > >>> + reg = readl_relaxed(usbmisc->base + id * 4); > >>> + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); > >> > >> #define IMX6_OTG_CTRL_USBMISC 1 << 7 > > yes. > >> > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static struct soc_data imx6q_data = { > >>> + .init = imx6q_usbmisc_init, > >>> +}; > >>> + > >>> + > >>> +static const struct of_device_id imx_usbmisc_dt_ids[] = { > >>> + { .compatible = "fsl,imx6q-usbmisc", .data = &imx6q_data }, > >> > >> something like this: > >> { .compatible = "fsl,imx6q-usbmisc", .data = &mx6_initialize_usb_hw }, > > It's designed not only used when initialization. > >> > >>> + { /* sentinel */ } > >>> +}; > >>> + > >>> +static int __devinit imx_usbmisc_probe(struct platform_device *pdev) > >>> +{ > >>> + struct resource *res; > >>> + struct usbmisc *data; > >>> + const struct of_device_id *of_id = > >>> + of_match_device(imx_usbmisc_dt_ids, &pdev->dev); > >>> + > >>> + int ret; > >>> + > >>> + if (usbmisc) > >>> + return -EBUSY; > >>> + > >>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > >>> + if (!data) > >>> + return -ENOMEM; > >>> + > >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> + data->base = devm_request_and_ioremap(&pdev->dev, res); > >>> + if (!data->base) > >>> + return -EADDRNOTAVAIL; > >>> + > >>> + data->clk = devm_clk_get(&pdev->dev, NULL); > >>> + if (IS_ERR(data->clk)) { > >>> + dev_err(&pdev->dev, > >>> + "failed to get clock, err=%ld\n", PTR_ERR(data->clk)); > >>> + return PTR_ERR(data->clk); > >>> + } > >>> + > >>> + ret = clk_prepare_enable(data->clk); > >>> + if (!ret) { > >> > >> Better: > >> if (ret) > >> return ret; > > To me, it does not make much difference. > > Not the the compiler, but IMHO the common pattern in the kernel is: > > ret = sensible_init(); > if (ret) > return ret; > > do_work(); > > >> > >>> + if (of_find_property(pdev->dev.of_node, > >>> + "fsl,disable-over-current", NULL)) > >>> + data->dis_oc = 1; > >>> + data->soc_data = of_id->data; > >>> + usbmisc = data; > >>> + } > >>> + > >> if (of_find_property(pdev->dev.of_node, > >> ... > >> > >>> + return ret; > >>> +} > >>> + > >>> +static int __devexit imx_usbmisc_remove(struct platform_device *pdev) > >>> +{ > >>> + clk_disable_unprepare(usbmisc->clk); > >>> + return 0; > >>> +} > >>> + > >>> +static struct platform_driver imx_usbmisc_driver = { > >>> + .probe = imx_usbmisc_probe, > >>> + .remove = __devexit_p(imx_usbmisc_remove), > >>> + .driver = { > >>> + .name = "imx_usbmisc", > >>> + .owner = THIS_MODULE, > >>> + .of_match_table = imx_usbmisc_dt_ids, > >>> + }, > >>> +}; > >>> + > >>> +int imx_usbmisc_init(struct device *usbdev) > >>> +{ > >>> + struct device_node *np = usbdev->of_node; > >>> + int ret; > >>> + > >>> + if (!usbmisc) > >>> + return 0; > >>> + > >>> + ret = of_alias_get_id(np, "usb"); > >>> + if (ret < 0) { > >>> + dev_err(usbdev, "failed to get alias id, errno %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + return usbmisc->soc_data->init(usbmisc, ret); > >>> +} > >>> +EXPORT_SYMBOL_GPL(imx_usbmisc_init); > >> > >> EXPORT_SYMBOL is only needed because it is > >> compliled as an extra kernel module. > > ci13xxx_imx may be module. > > Why make the usb_misc an independent module, why not just link it into > the ci13xxx_imx.ko Yes Thanks Richard > > Marc > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | >
On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: > i.MX usb controllers shares non-core registers, which may include > SoC specific controls. We take it as a usbmisc device and usbmisc > driver export functions needed by ci13xxx_imx driver. > > For example, Sabrelite board has bad over-current design, we can > usbmisc to disable over-current detect. > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > --- > .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ > drivers/usb/chipidea/Makefile | 2 +- > drivers/usb/chipidea/ci13xxx_imx.c | 4 + > drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ > 4 files changed, 164 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt > create mode 100644 drivers/usb/chipidea/imx_usbmisc.c > > diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > new file mode 100644 > index 0000000..09f01ca > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > @@ -0,0 +1,15 @@ > +* Freescale i.MX non-core registers > + > +Required properties: > +- compatible: Should be "fsl,imx6q-usbmisc" > +- reg: Should contain registers location and length > + > +Optional properties: > +- fsl,disable-over-current: disable over current detect > + > +Examples: > +usbmisc@02184800 { > + compatible = "fsl,imx6q-usbmisc"; > + reg = <0x02184800 0x200>; > + fsl,disable-over-current; > +}; > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 3f56b76..46fc31c 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) > endif > > ifneq ($(CONFIG_OF_DEVICE),) > - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o > + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o > endif > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > index b3173d8..dd7f3a3 100644 > --- a/drivers/usb/chipidea/ci13xxx_imx.c > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > @@ -24,6 +24,8 @@ > > #include "ci.h" > > +int imx_usbmisc_init(struct device *usbdev); > + > #define pdev_to_phy(pdev) \ > ((struct usb_phy *)platform_get_drvdata(pdev)) > #define ci_to_imx_data(ci) \ > @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); > } > > + imx_usbmisc_init(&pdev->dev); > + > platform_set_drvdata(pdev, data); > > plat_ci = ci13xxx_add_device(&pdev->dev, > diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c > new file mode 100644 > index 0000000..33a8ec0 > --- /dev/null > +++ b/drivers/usb/chipidea/imx_usbmisc.c > @@ -0,0 +1,144 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <asm/io.h> linux/io.h > + > +struct usbmisc; > + > +struct soc_data { > + int (*init) (struct usbmisc *usbmisc, int id); > + void (*exit) (struct usbmisc *usbmisc, int id); > +}; > + > +struct usbmisc { > + struct soc_data *soc_data; > + void __iomem *base; > + struct clk *clk; > + > + int dis_oc:1; > +}; > + > +/* Since we only have one usbmisc device at runtime, we global variables */ > +static struct usbmisc *usbmisc; NACK What you've done here exactly matches your current usecase but is not enough for any of the other usecases I can think of. Even the i.MX6 has three USB ports, each of them has a overcurrent disable bit. Also, there are more flags, like: - use internal phy - power pin polarity - ttl enabled I think the best you can do here is to add the flags to the fsl,imx6q-usb device nodes: usb@02184000 { /* USB OTG */ compatible = "fsl,imx6q-usb", "fsl,imx27-usb"; reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; status = "disabled"; fsl,disable-over-current; <other flags>; }; Then add a usbmisc device which does not contain any flags. During initialization of the fsl,imx6q-usb device you can then pass a pointer to its device node to imx_usbmisc_init. Sascha
On Mon, Jul 16, 2012 at 10:25:20AM +0200, Sascha Hauer wrote: > On Thu, Jul 12, 2012 at 03:01:52PM +0800, Richard Zhao wrote: > > i.MX usb controllers shares non-core registers, which may include > > SoC specific controls. We take it as a usbmisc device and usbmisc > > driver export functions needed by ci13xxx_imx driver. > > > > For example, Sabrelite board has bad over-current design, we can > > usbmisc to disable over-current detect. > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > > --- > > .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ > > drivers/usb/chipidea/Makefile | 2 +- > > drivers/usb/chipidea/ci13xxx_imx.c | 4 + > > drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ > > 4 files changed, 164 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > create mode 100644 drivers/usb/chipidea/imx_usbmisc.c > > > > diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > new file mode 100644 > > index 0000000..09f01ca > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt > > @@ -0,0 +1,15 @@ > > +* Freescale i.MX non-core registers > > + > > +Required properties: > > +- compatible: Should be "fsl,imx6q-usbmisc" > > +- reg: Should contain registers location and length > > + > > +Optional properties: > > +- fsl,disable-over-current: disable over current detect > > + > > +Examples: > > +usbmisc@02184800 { > > + compatible = "fsl,imx6q-usbmisc"; > > + reg = <0x02184800 0x200>; > > + fsl,disable-over-current; > > +}; > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > > index 3f56b76..46fc31c 100644 > > --- a/drivers/usb/chipidea/Makefile > > +++ b/drivers/usb/chipidea/Makefile > > @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) > > endif > > > > ifneq ($(CONFIG_OF_DEVICE),) > > - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o > > + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o > > endif > > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > > index b3173d8..dd7f3a3 100644 > > --- a/drivers/usb/chipidea/ci13xxx_imx.c > > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > > @@ -24,6 +24,8 @@ > > > > #include "ci.h" > > > > +int imx_usbmisc_init(struct device *usbdev); > > + > > #define pdev_to_phy(pdev) \ > > ((struct usb_phy *)platform_get_drvdata(pdev)) > > #define ci_to_imx_data(ci) \ > > @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > > dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); > > } > > > > + imx_usbmisc_init(&pdev->dev); > > + > > platform_set_drvdata(pdev, data); > > > > plat_ci = ci13xxx_add_device(&pdev->dev, > > diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c > > new file mode 100644 > > index 0000000..33a8ec0 > > --- /dev/null > > +++ b/drivers/usb/chipidea/imx_usbmisc.c > > @@ -0,0 +1,144 @@ > > +/* > > + * Copyright 2012 Freescale Semiconductor, Inc. > > + * > > + * The code contained herein is licensed under the GNU General Public > > + * License. You may obtain a copy of the GNU General Public License > > + * Version 2 or later at the following locations: > > + * > > + * http://www.opensource.org/licenses/gpl-license.html > > + * http://www.gnu.org/copyleft/gpl.html > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/of_platform.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <asm/io.h> > > linux/io.h > > > + > > +struct usbmisc; > > + > > +struct soc_data { > > + int (*init) (struct usbmisc *usbmisc, int id); > > + void (*exit) (struct usbmisc *usbmisc, int id); > > +}; > > + > > +struct usbmisc { > > + struct soc_data *soc_data; > > + void __iomem *base; > > + struct clk *clk; > > + > > + int dis_oc:1; > > +}; > > + > > +/* Since we only have one usbmisc device at runtime, we global variables */ > > +static struct usbmisc *usbmisc; > > NACK Right, it's a bad design. I'm considering change it too. Maybe I will move it out this patch series. As I asked you in another mail in this thread, do you think it's good to put it here or in mach-imx/ ? > > What you've done here exactly matches your current usecase but is not > enough for any of the other usecases I can think of. Even the i.MX6 has > three USB ports, each of them has a overcurrent disable bit. Also, there > are more flags, like: Yes, I'll change it to support different usb with different properties. > > - use internal phy > - power pin polarity > - ttl enabled I focus on imx6 now. So I think the property can be added when it's needed. Now I only use disable oc. > > I think the best you can do here is to add the flags to the > fsl,imx6q-usb device nodes: > > usb@02184000 { /* USB OTG */ > compatible = "fsl,imx6q-usb", "fsl,imx27-usb"; > reg = <0x02184000 0x200>; > interrupts = <0 43 0x04>; > fsl,usbphy = <&usbphy1>; > status = "disabled"; > fsl,disable-over-current; > <other flags>; > }; > > Then add a usbmisc device which does not contain any flags. During > initialization of the fsl,imx6q-usb device you can then pass a pointer > to its device node to imx_usbmisc_init. Good suggestion. Thanks Richard > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
On Mon, Jul 16, 2012 at 04:38:38PM +0800, Richard Zhao wrote: > > > > NACK > Right, it's a bad design. I'm considering change it too. Maybe I will > move it out this patch series. As I asked you in another mail in this > thread, do you think it's good to put it here or in mach-imx/ ? I prefer it to be in drivers/usb/chipidea. > > > > What you've done here exactly matches your current usecase but is not > > enough for any of the other usecases I can think of. Even the i.MX6 has > > three USB ports, each of them has a overcurrent disable bit. Also, there > > are more flags, like: > Yes, I'll change it to support different usb with different properties. > > > > - use internal phy > > - power pin polarity > > - ttl enabled > I focus on imx6 now. So I think the property can be added when it's > needed. Now I only use disable oc. That's fine, but you should give the impression that you thought about other usecases and other SoCs than your current one, it'll make it easier for us to believe that your patch does the right thing ;) Sascha
Dear Richard Zhao, [...] > +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) > +{ > + u32 reg; > + > + if (usbmisc->dis_oc) { > + reg = readl_relaxed(usbmisc->base + id * 4); > + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); 1<< 7 ? This yearns for fixing with proper #defined value. [...] The rest of the patches looks OK, you can add my Reviewed-By: <marex@denx.de> to those. This one though will need further investigation. Best regards, Marek Vasut
diff --git a/Documentation/devicetree/bindings/usb/imx-usbmisc.txt b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt new file mode 100644 index 0000000..09f01ca --- /dev/null +++ b/Documentation/devicetree/bindings/usb/imx-usbmisc.txt @@ -0,0 +1,15 @@ +* Freescale i.MX non-core registers + +Required properties: +- compatible: Should be "fsl,imx6q-usbmisc" +- reg: Should contain registers location and length + +Optional properties: +- fsl,disable-over-current: disable over current detect + +Examples: +usbmisc@02184800 { + compatible = "fsl,imx6q-usbmisc"; + reg = <0x02184800 0x200>; + fsl,disable-over-current; +}; diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile index 3f56b76..46fc31c 100644 --- a/drivers/usb/chipidea/Makefile +++ b/drivers/usb/chipidea/Makefile @@ -17,5 +17,5 @@ ifneq ($(CONFIG_PCI),) endif ifneq ($(CONFIG_OF_DEVICE),) - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o imx_usbmisc.o endif diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c index b3173d8..dd7f3a3 100644 --- a/drivers/usb/chipidea/ci13xxx_imx.c +++ b/drivers/usb/chipidea/ci13xxx_imx.c @@ -24,6 +24,8 @@ #include "ci.h" +int imx_usbmisc_init(struct device *usbdev); + #define pdev_to_phy(pdev) \ ((struct usb_phy *)platform_get_drvdata(pdev)) #define ci_to_imx_data(ci) \ @@ -142,6 +144,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); } + imx_usbmisc_init(&pdev->dev); + platform_set_drvdata(pdev, data); plat_ci = ci13xxx_add_device(&pdev->dev, diff --git a/drivers/usb/chipidea/imx_usbmisc.c b/drivers/usb/chipidea/imx_usbmisc.c new file mode 100644 index 0000000..33a8ec0 --- /dev/null +++ b/drivers/usb/chipidea/imx_usbmisc.c @@ -0,0 +1,144 @@ +/* + * Copyright 2012 Freescale Semiconductor, Inc. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <asm/io.h> + +struct usbmisc; + +struct soc_data { + int (*init) (struct usbmisc *usbmisc, int id); + void (*exit) (struct usbmisc *usbmisc, int id); +}; + +struct usbmisc { + struct soc_data *soc_data; + void __iomem *base; + struct clk *clk; + + int dis_oc:1; +}; + +/* Since we only have one usbmisc device at runtime, we global variables */ +static struct usbmisc *usbmisc; + +static int imx6q_usbmisc_init(struct usbmisc *usbmisc, int id) +{ + u32 reg; + + if (usbmisc->dis_oc) { + reg = readl_relaxed(usbmisc->base + id * 4); + writel_relaxed(reg | (1 << 7), usbmisc->base + id * 4); + } + + return 0; +} + +static struct soc_data imx6q_data = { + .init = imx6q_usbmisc_init, +}; + + +static const struct of_device_id imx_usbmisc_dt_ids[] = { + { .compatible = "fsl,imx6q-usbmisc", .data = &imx6q_data }, + { /* sentinel */ } +}; + +static int __devinit imx_usbmisc_probe(struct platform_device *pdev) +{ + struct resource *res; + struct usbmisc *data; + const struct of_device_id *of_id = + of_match_device(imx_usbmisc_dt_ids, &pdev->dev); + + int ret; + + if (usbmisc) + return -EBUSY; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + data->base = devm_request_and_ioremap(&pdev->dev, res); + if (!data->base) + return -EADDRNOTAVAIL; + + data->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(data->clk)) { + dev_err(&pdev->dev, + "failed to get clock, err=%ld\n", PTR_ERR(data->clk)); + return PTR_ERR(data->clk); + } + + ret = clk_prepare_enable(data->clk); + if (!ret) { + if (of_find_property(pdev->dev.of_node, + "fsl,disable-over-current", NULL)) + data->dis_oc = 1; + data->soc_data = of_id->data; + usbmisc = data; + } + + return ret; +} + +static int __devexit imx_usbmisc_remove(struct platform_device *pdev) +{ + clk_disable_unprepare(usbmisc->clk); + return 0; +} + +static struct platform_driver imx_usbmisc_driver = { + .probe = imx_usbmisc_probe, + .remove = __devexit_p(imx_usbmisc_remove), + .driver = { + .name = "imx_usbmisc", + .owner = THIS_MODULE, + .of_match_table = imx_usbmisc_dt_ids, + }, +}; + +int imx_usbmisc_init(struct device *usbdev) +{ + struct device_node *np = usbdev->of_node; + int ret; + + if (!usbmisc) + return 0; + + ret = of_alias_get_id(np, "usb"); + if (ret < 0) { + dev_err(usbdev, "failed to get alias id, errno %d\n", ret); + return ret; + } + + return usbmisc->soc_data->init(usbmisc, ret); +} +EXPORT_SYMBOL_GPL(imx_usbmisc_init); + +static int __init imx_usbmisc_drv_init(void) +{ + return platform_driver_register(&imx_usbmisc_driver); +} +subsys_initcall(imx_usbmisc_drv_init); + +static void __exit imx_usbmisc_drv_exit(void) +{ + platform_driver_unregister(&imx_usbmisc_driver); +} +module_exit(imx_usbmisc_drv_exit); + +MODULE_LICENSE("GPL v2");
i.MX usb controllers shares non-core registers, which may include SoC specific controls. We take it as a usbmisc device and usbmisc driver export functions needed by ci13xxx_imx driver. For example, Sabrelite board has bad over-current design, we can usbmisc to disable over-current detect. Signed-off-by: Richard Zhao <richard.zhao@freescale.com> --- .../devicetree/bindings/usb/imx-usbmisc.txt | 15 ++ drivers/usb/chipidea/Makefile | 2 +- drivers/usb/chipidea/ci13xxx_imx.c | 4 + drivers/usb/chipidea/imx_usbmisc.c | 144 ++++++++++++++++++++ 4 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/usb/imx-usbmisc.txt create mode 100644 drivers/usb/chipidea/imx_usbmisc.c