| Message ID | 1279815922-27198-3-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Grant Likely |
| Headers | show |
On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@denx.de> wrote: > The driver creates platform devices based on the information > from USB nodes in the flat device tree. This is the replacement > for old arch fsl_soc usb code removed by the previous patch. > It uses usual of-style binding, available EHCI-HCD and UDC > drivers can be bound to the created devices. The new of-style > driver additionaly instantiates USB OTG platform device, as the > appropriate USB OTG driver will be added soon. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > Cc: Kumar Gala <galak@kernel.crashing.org> > Cc: Grant Likely <grant.likely@secretlab.ca> Hi Anatolij, Looks pretty good, but some comments below. g. > --- > drivers/usb/gadget/Kconfig | 1 + > drivers/usb/host/Kconfig | 5 + > drivers/usb/host/Makefile | 1 + > drivers/usb/host/fsl-mph-dr-of.c | 189 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 196 insertions(+), 0 deletions(-) > create mode 100644 drivers/usb/host/fsl-mph-dr-of.c > > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig > index cd27f9b..e15e314 100644 > --- a/drivers/usb/gadget/Kconfig > +++ b/drivers/usb/gadget/Kconfig > @@ -158,6 +158,7 @@ config USB_GADGET_FSL_USB2 > boolean "Freescale Highspeed USB DR Peripheral Controller" > depends on FSL_SOC || ARCH_MXC > select USB_GADGET_DUALSPEED > + select USB_FSL_MPH_DR_OF > help > Some of Freescale PowerPC processors have a High Speed > Dual-Role(DR) USB controller, which supports device mode. > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 2d926ce..6687523 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX > support both high speed and full speed devices, or high speed > devices only. > > +config USB_FSL_MPH_DR_OF > + bool > + depends on PPC_OF Drop the depends. This is selected by USB_EHCI_FSL and USB_GADGET_FSL_USB2, which are already PPC only. > + > config USB_EHCI_FSL > bool "Support for Freescale on-chip EHCI USB controller" > depends on USB_EHCI_HCD && FSL_SOC > select USB_EHCI_ROOT_HUB_TT > + select USB_FSL_MPH_DR_OF > ---help--- > Variation of ARC USB block used in some Freescale chips. > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > index b6315aa..aacbe82 100644 > --- a/drivers/usb/host/Makefile > +++ b/drivers/usb/host/Makefile > @@ -33,4 +33,5 @@ obj-$(CONFIG_USB_R8A66597_HCD) += r8a66597-hcd.o > obj-$(CONFIG_USB_ISP1760_HCD) += isp1760.o > obj-$(CONFIG_USB_HWA_HCD) += hwa-hc.o > obj-$(CONFIG_USB_IMX21_HCD) += imx21-hcd.o > +obj-$(CONFIG_USB_FSL_MPH_DR_OF) += fsl-mph-dr-of.o > > diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c > new file mode 100644 > index 0000000..020a939 > --- /dev/null > +++ b/drivers/usb/host/fsl-mph-dr-of.c > @@ -0,0 +1,189 @@ > +/* > + * Setup platform devices needed by the Freescale multi-port host > + * and/or dual-role USB controller modules based on the description > + * in flat device tree. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/fsl_devices.h> > +#include <linux/io.h> > +#include <linux/of_platform.h> > + > +struct fsl_usb2_dev_data { > + char *dr_mode; /* controller mode */ > + char *drivers[3]; /* drivers to instantiate for this mode */ > + enum fsl_usb2_operating_modes op_mode; /* operating mode */ > +}; > + > +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = { > + { > + "host", > + { "fsl-ehci", NULL, NULL, }, > + FSL_USB2_DR_HOST, > + }, > + { > + "otg", > + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", }, > + FSL_USB2_DR_OTG, > + }, > + { > + "periferal", spelling. "peripheral" > + { "fsl-usb2-udc", NULL, NULL, }, > + FSL_USB2_DR_DEVICE, > + }, > +}; Program defensively. Use the following form: + { + .dr_mode = "host", + .drivers = { "fsl-ehci", NULL, NULL, }, + .op_mode = FSL_USB2_DR_HOST, + }, > + > +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np) > +{ > + const unsigned char *prop; > + int i; > + > + prop = of_get_property(np, "dr_mode", NULL); > + if (prop) { > + for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) { > + if (!strcmp(prop, dr_mode_data[i].dr_mode)) > + return &dr_mode_data[i]; > + } Print a warning if you get through the loop, but don't match anything. That means that dr_mode has a bad value. > + } > + return &dr_mode_data[0]; /* mode not specified, use host */ > +} > + > +static enum fsl_usb2_phy_modes __devinit determine_usb_phy(const char *phy_type) > +{ > + if (!phy_type) > + return FSL_USB2_PHY_NONE; > + if (!strcasecmp(phy_type, "ulpi")) > + return FSL_USB2_PHY_ULPI; > + if (!strcasecmp(phy_type, "utmi")) > + return FSL_USB2_PHY_UTMI; > + if (!strcasecmp(phy_type, "utmi_wide")) > + return FSL_USB2_PHY_UTMI_WIDE; > + if (!strcasecmp(phy_type, "serial")) > + return FSL_USB2_PHY_SERIAL; > + > + return FSL_USB2_PHY_NONE; > +} > + > +struct platform_device * __devinit > +fsl_usb2_device_register(struct of_device *ofdev, > + struct fsl_usb2_platform_data *pdata, > + const char *name, int id) In general, it is better to have the function name on the same line as the return arguements so that grep shows the relevant info. Move the arguments to subsequent lines. > +{ > + struct platform_device *pdev; > + const struct resource *res = ofdev->resource; > + unsigned int num = ofdev->num_resources; > + int retval; > + > + pdev = platform_device_alloc(name, id); > + if (!pdev) { > + retval = -ENOMEM; > + goto error; > + } > + > + pdev->dev.parent = &ofdev->dev; Good! The sub-devices show up sanely in the device hierarchy by setting the parent pointer. > + > + pdev->dev.coherent_dma_mask = ofdev->dev.coherent_dma_mask; > + pdev->dev.dma_mask = &pdev->archdata.dma_mask; > + *pdev->dev.dma_mask = *ofdev->dev.dma_mask; > + > + retval = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > + if (retval) > + goto error; > + > + if (num) { > + retval = platform_device_add_resources(pdev, res, num); > + if (retval) > + goto error; > + } > + > + retval = platform_device_add(pdev); > + if (retval) > + goto error; > + > + return pdev; > + > +error: > + platform_device_put(pdev); > + return ERR_PTR(retval); > +} > + > +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev, > + const struct of_device_id *match) > +{ > + struct device_node *np = ofdev->dev.of_node; > + struct platform_device *usb_dev; > + struct fsl_usb2_platform_data data, *pdata; > + struct fsl_usb2_dev_data *dev_data; > + const unsigned char *prop; > + static unsigned int idx; > + int i; > + > + if (!of_device_is_available(np)) > + return -ENODEV; What is this for? > + > + pdata = match->data; > + if (!pdata) { The match table doesn't have any data, so this is a no-op. > + memset(&data, 0, sizeof(data)); > + pdata = &data; > + } > + > + dev_data = get_dr_mode_data(np); > + > + if (of_device_is_compatible(np, "fsl-usb2-mph")) { > + prop = of_get_property(np, "port0", NULL); > + if (prop) if (of_get_property()) > + pdata->port_enables |= FSL_USB2_PORT0_ENABLED; > + > + prop = of_get_property(np, "port1", NULL); > + if (prop) Ditto > + pdata->port_enables |= FSL_USB2_PORT1_ENABLED; > + > + pdata->operating_mode = FSL_USB2_MPH_HOST; > + } else { > + /* setup mode selected in the device tree */ > + pdata->operating_mode = dev_data->op_mode; > + } > + > + prop = of_get_property(np, "phy_type", NULL); > + pdata->phy_mode = determine_usb_phy(prop); > + > + for (i = 0; i < ARRAY_SIZE(dev_data->drivers); i++) { > + if (!dev_data->drivers[i]) > + continue; > + usb_dev = fsl_usb2_device_register(ofdev, pdata, > + dev_data->drivers[i], idx); > + if (IS_ERR(usb_dev)) { > + dev_err(&ofdev->dev, "Can't register usb device\n"); > + return PTR_ERR(usb_dev); > + } > + } > + idx++; > + return 0; > +} > + > +static const struct of_device_id fsl_usb2_mph_dr_of_match[] = { > + { .compatible = "fsl-usb2-mph", }, > + { .compatible = "fsl-usb2-dr", }, > + {}, > +}; > + > +static struct of_platform_driver fsl_usb2_mph_dr_driver = { > + .driver = { > + .name = "fsl-usb2-mph-dr", > + .owner = THIS_MODULE, > + .of_match_table = fsl_usb2_mph_dr_of_match, > + }, > + .probe = fsl_usb2_mph_dr_of_probe, > +}; No remove hook? > + > +static int __init fsl_usb2_mph_dr_init(void) > +{ > + return of_register_platform_driver(&fsl_usb2_mph_dr_driver); > +} > +fs_initcall(fsl_usb2_mph_dr_init); Why fs_initcall? Is module_init() not sufficient? > -- > 1.7.0.4 > >
On Wed, Jul 28, 2010 at 02:16:08AM -0600, Grant Likely wrote: [...] > > +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev, > > + const struct of_device_id *match) > > +{ > > + struct device_node *np = ofdev->dev.of_node; > > + struct platform_device *usb_dev; > > + struct fsl_usb2_platform_data data, *pdata; > > + struct fsl_usb2_dev_data *dev_data; > > + const unsigned char *prop; > > + static unsigned int idx; > > + int i; > > + > > + if (!of_device_is_available(np)) > > + return -ENODEV; > > What is this for? USB pins/clocks might be muxed away to other peripherals, like eSDHC. In such cases firmware marks USB as unavailable (status = "disabled"). If you try to access USB while it is disabled the SOC will hang.
Hi Grant, On Wed, 28 Jul 2010 02:16:08 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@denx.de> wrote: > > The driver creates platform devices based on the information > > from USB nodes in the flat device tree. This is the replacement > > for old arch fsl_soc usb code removed by the previous patch. > > It uses usual of-style binding, available EHCI-HCD and UDC > > drivers can be bound to the created devices. The new of-style > > driver additionaly instantiates USB OTG platform device, as the > > appropriate USB OTG driver will be added soon. > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > Cc: Kumar Gala <galak@kernel.crashing.org> > > Cc: Grant Likely <grant.likely@secretlab.ca> > > Hi Anatolij, > > Looks pretty good, but some comments below. Thanks for review and comments! Greg already merged this series and therefore I'll submit an incremental cleanup patch to address outstanding issues. My reply is below. ... > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX > > support both high speed and full speed devices, or high speed > > devices only. > > > > +config USB_FSL_MPH_DR_OF > > + bool > > + depends on PPC_OF > > Drop the depends. This is selected by USB_EHCI_FSL and > USB_GADGET_FSL_USB2, which are already PPC only. Okay, will remove it. ... > > +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = { > > + { > > + "host", > > + { "fsl-ehci", NULL, NULL, }, > > + FSL_USB2_DR_HOST, > > + }, > > + { > > + "otg", > > + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", }, > > + FSL_USB2_DR_OTG, > > + }, > > + { > > + "periferal", > > spelling. "peripheral" Right, thanks for catching! Will fix it. > > > + { "fsl-usb2-udc", NULL, NULL, }, > > + FSL_USB2_DR_DEVICE, > > + }, > > +}; > > Program defensively. Use the following form: > + { > + .dr_mode = "host", > + .drivers = { "fsl-ehci", NULL, NULL, }, > + .op_mode = FSL_USB2_DR_HOST, > + }, I'll change it too as suggested. ... > > +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np) > > +{ > > + const unsigned char *prop; > > + int i; > > + > > + prop = of_get_property(np, "dr_mode", NULL); > > + if (prop) { > > + for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) { > > + if (!strcmp(prop, dr_mode_data[i].dr_mode)) > > + return &dr_mode_data[i]; > > + } > > Print a warning if you get through the loop, but don't match anything. > That means that dr_mode has a bad value. I'll add a warning here. ... > > +struct platform_device * __devinit > > +fsl_usb2_device_register(struct of_device *ofdev, > > + struct fsl_usb2_platform_data *pdata, > > + const char *name, int id) > > In general, it is better to have the function name on the same line as > the return arguements so that grep shows the relevant info. Move the > arguments to subsequent lines. It will be fixed in a clean-up patch. ... > > +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev, > > + const struct of_device_id *match) > > +{ > > + struct device_node *np = ofdev->dev.of_node; > > + struct platform_device *usb_dev; > > + struct fsl_usb2_platform_data data, *pdata; > > + struct fsl_usb2_dev_data *dev_data; > > + const unsigned char *prop; > > + static unsigned int idx; > > + int i; > > + > > + if (!of_device_is_available(np)) > > + return -ENODEV; > > What is this for? Anton already answered this question better than I could do. > > + > > + pdata = match->data; > > + if (!pdata) { > > The match table doesn't have any data, so this is a no-op. The next patch [PATCH 3/3] of the series adds the data to the match table. ... > > + if (of_device_is_compatible(np, "fsl-usb2-mph")) { > > + prop = of_get_property(np, "port0", NULL); > > + if (prop) > > if (of_get_property()) > > > + pdata->port_enables |= FSL_USB2_PORT0_ENABLED; > > + > > + prop = of_get_property(np, "port1", NULL); > > + if (prop) > > Ditto Okay, I'll clean this up. ... > > +static struct of_platform_driver fsl_usb2_mph_dr_driver = { > > + .driver = { > > + .name = "fsl-usb2-mph-dr", > > + .owner = THIS_MODULE, > > + .of_match_table = fsl_usb2_mph_dr_of_match, > > + }, > > + .probe = fsl_usb2_mph_dr_of_probe, > > +}; > > No remove hook? Since the purpose of the driver is only creation of platform devices according to the selected dual role controller mode described in the device tree, I do not see much sense of making the driver a module and provide a remove hook for destruction of created devices. ... > > +static int __init fsl_usb2_mph_dr_init(void) > > +{ > > + return of_register_platform_driver(&fsl_usb2_mph_dr_driver); > > +} > > +fs_initcall(fsl_usb2_mph_dr_init); > > Why fs_initcall? Is module_init() not sufficient? No. Using module_init() here results in initializing the driver after loading FLS USB OTG and EHCI-FSL drivers and reverted probing: probe() in EHCI-FSL driver is called before probe() in OTG driver and doesn't find OTG transceiver resource which is set and exported by probe() in OTG driver. This breaks both USB host and device controller support if dual role controller is configured to operate in OTG mode. Thanks, Anatolij
On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin <agust@denx.de> wrote: > Hi Grant, > > On Wed, 28 Jul 2010 02:16:08 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > >> On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@denx.de> wrote: >> > The driver creates platform devices based on the information >> > from USB nodes in the flat device tree. This is the replacement >> > for old arch fsl_soc usb code removed by the previous patch. >> > It uses usual of-style binding, available EHCI-HCD and UDC >> > drivers can be bound to the created devices. The new of-style >> > driver additionaly instantiates USB OTG platform device, as the >> > appropriate USB OTG driver will be added soon. >> > >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> >> > Cc: Kumar Gala <galak@kernel.crashing.org> >> > Cc: Grant Likely <grant.likely@secretlab.ca> >> >> Hi Anatolij, >> >> Looks pretty good, but some comments below. > > Thanks for review and comments! Greg already merged this series and > therefore I'll submit an incremental cleanup patch to address > outstanding issues. My reply is below. Greg maintains a patchwork tree IIRC. I believe he drops patches from his linux-next branch if they need to be reworked. Greg, do I have this right? Also, my preference would be to see some 3rd party testing before committing to having this merged. > > ... >> > --- a/drivers/usb/host/Kconfig >> > +++ b/drivers/usb/host/Kconfig >> > @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX >> > support both high speed and full speed devices, or high speed >> > devices only. >> > >> > +config USB_FSL_MPH_DR_OF >> > + bool >> > + depends on PPC_OF >> >> Drop the depends. This is selected by USB_EHCI_FSL and >> USB_GADGET_FSL_USB2, which are already PPC only. > > Okay, will remove it. > > ... >> > +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = { >> > + { >> > + "host", >> > + { "fsl-ehci", NULL, NULL, }, >> > + FSL_USB2_DR_HOST, >> > + }, >> > + { >> > + "otg", >> > + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", }, >> > + FSL_USB2_DR_OTG, >> > + }, >> > + { >> > + "periferal", >> >> spelling. "peripheral" > > Right, thanks for catching! Will fix it. > >> >> > + { "fsl-usb2-udc", NULL, NULL, }, >> > + FSL_USB2_DR_DEVICE, >> > + }, >> > +}; >> >> Program defensively. Use the following form: >> + { >> + .dr_mode = "host", >> + .drivers = { "fsl-ehci", NULL, NULL, }, >> + .op_mode = FSL_USB2_DR_HOST, >> + }, > > I'll change it too as suggested. > > ... >> > +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np) >> > +{ >> > + const unsigned char *prop; >> > + int i; >> > + >> > + prop = of_get_property(np, "dr_mode", NULL); >> > + if (prop) { >> > + for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) { >> > + if (!strcmp(prop, dr_mode_data[i].dr_mode)) >> > + return &dr_mode_data[i]; >> > + } >> >> Print a warning if you get through the loop, but don't match anything. >> That means that dr_mode has a bad value. > > I'll add a warning here. > > ... >> > +struct platform_device * __devinit >> > +fsl_usb2_device_register(struct of_device *ofdev, >> > + struct fsl_usb2_platform_data *pdata, >> > + const char *name, int id) >> >> In general, it is better to have the function name on the same line as >> the return arguements so that grep shows the relevant info. Move the >> arguments to subsequent lines. > > It will be fixed in a clean-up patch. > > ... >> > +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev, >> > + const struct of_device_id *match) >> > +{ >> > + struct device_node *np = ofdev->dev.of_node; >> > + struct platform_device *usb_dev; >> > + struct fsl_usb2_platform_data data, *pdata; >> > + struct fsl_usb2_dev_data *dev_data; >> > + const unsigned char *prop; >> > + static unsigned int idx; >> > + int i; >> > + >> > + if (!of_device_is_available(np)) >> > + return -ENODEV; >> >> What is this for? > > Anton already answered this question better than I could do. > >> > + >> > + pdata = match->data; >> > + if (!pdata) { >> >> The match table doesn't have any data, so this is a no-op. > > The next patch [PATCH 3/3] of the series adds the data to the > match table. > > ... >> > + if (of_device_is_compatible(np, "fsl-usb2-mph")) { >> > + prop = of_get_property(np, "port0", NULL); >> > + if (prop) >> >> if (of_get_property()) >> >> > + pdata->port_enables |= FSL_USB2_PORT0_ENABLED; >> > + >> > + prop = of_get_property(np, "port1", NULL); >> > + if (prop) >> >> Ditto > > Okay, I'll clean this up. > > ... >> > +static struct of_platform_driver fsl_usb2_mph_dr_driver = { >> > + .driver = { >> > + .name = "fsl-usb2-mph-dr", >> > + .owner = THIS_MODULE, >> > + .of_match_table = fsl_usb2_mph_dr_of_match, >> > + }, >> > + .probe = fsl_usb2_mph_dr_of_probe, >> > +}; >> >> No remove hook? > > Since the purpose of the driver is only creation of platform devices > according to the selected dual role controller mode described in > the device tree, I do not see much sense of making the driver a module > and provide a remove hook for destruction of created devices. It is still appropriate to have a remove function so the driver can be unbound. The remove function should unregister the platform devices it created in .probe(). > > ... >> > +static int __init fsl_usb2_mph_dr_init(void) >> > +{ >> > + return of_register_platform_driver(&fsl_usb2_mph_dr_driver); >> > +} >> > +fs_initcall(fsl_usb2_mph_dr_init); >> >> Why fs_initcall? Is module_init() not sufficient? > > No. Using module_init() here results in initializing the driver after > loading FLS USB OTG and EHCI-FSL drivers and reverted probing: probe() > in EHCI-FSL driver is called before probe() in OTG driver and doesn't > find OTG transceiver resource which is set and exported by probe() in > OTG driver. This breaks both USB host and device controller support > if dual role controller is configured to operate in OTG mode. This is still broken then, and it is just luck that it is even working now. The three sub-devices are registered in sequence which should handle your ordering issues if you put the OTG driver at the start of the list; but even that is rather hacky. You need to make the EHCI-FSL drivers defer initialization if it has to wait for the OTG driver. One option might be to use a bus notifier for this. I won't nack this patch because of this problem because the problem already exists in the current code, but it really does need to be fixed. g.
On Wed, Jul 28, 2010 at 12:14:14PM -0600, Grant Likely wrote: > On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin <agust@denx.de> wrote: > > Hi Grant, > > > > On Wed, 28 Jul 2010 02:16:08 -0600 > > Grant Likely <grant.likely@secretlab.ca> wrote: > > > >> On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@denx.de> wrote: > >> > The driver creates platform devices based on the information > >> > from USB nodes in the flat device tree. This is the replacement > >> > for old arch fsl_soc usb code removed by the previous patch. > >> > It uses usual of-style binding, available EHCI-HCD and UDC > >> > drivers can be bound to the created devices. The new of-style > >> > driver additionaly instantiates USB OTG platform device, as the > >> > appropriate USB OTG driver will be added soon. > >> > > >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> > >> > Cc: Kumar Gala <galak@kernel.crashing.org> > >> > Cc: Grant Likely <grant.likely@secretlab.ca> > >> > >> Hi Anatolij, > >> > >> Looks pretty good, but some comments below. > > > > Thanks for review and comments! Greg already merged this series and > > therefore I'll submit an incremental cleanup patch to address > > outstanding issues. My reply is below. > > Greg maintains a patchwork tree IIRC. I believe he drops patches from > his linux-next branch if they need to be reworked. Greg, do I have > this right? Yup. I can easily drop all of these patches. > Also, my preference would be to see some 3rd party testing before > committing to having this merged. Ok, I will go drop them all now, and wait for some that pass your review. thanks, greg k-h
On Wed, Jul 28, 2010 at 11:46:47AM -0700, Greg KH wrote: > On Wed, Jul 28, 2010 at 12:14:14PM -0600, Grant Likely wrote: > > On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin <agust@denx.de> wrote: > > > Hi Grant, > > > > > > On Wed, 28 Jul 2010 02:16:08 -0600 > > > Grant Likely <grant.likely@secretlab.ca> wrote: > > > > > >> On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@denx.de> wrote: > > >> > The driver creates platform devices based on the information > > >> > from USB nodes in the flat device tree. This is the replacement > > >> > for old arch fsl_soc usb code removed by the previous patch. > > >> > It uses usual of-style binding, available EHCI-HCD and UDC > > >> > drivers can be bound to the created devices. The new of-style > > >> > driver additionaly instantiates USB OTG platform device, as the > > >> > appropriate USB OTG driver will be added soon. > > >> > > > >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > >> > Cc: Kumar Gala <galak@kernel.crashing.org> > > >> > Cc: Grant Likely <grant.likely@secretlab.ca> > > >> > > >> Hi Anatolij, > > >> > > >> Looks pretty good, but some comments below. > > > > > > Thanks for review and comments! Greg already merged this series and > > > therefore I'll submit an incremental cleanup patch to address > > > outstanding issues. My reply is below. > > > > Greg maintains a patchwork tree IIRC. I believe he drops patches from > > his linux-next branch if they need to be reworked. Greg, do I have > > this right? > > Yup. I can easily drop all of these patches. > > > Also, my preference would be to see some 3rd party testing before > > committing to having this merged. > > Ok, I will go drop them all now, and wait for some that pass your > review. All 3 now dropped. thanks, greg k-h
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index cd27f9b..e15e314 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -158,6 +158,7 @@ config USB_GADGET_FSL_USB2 boolean "Freescale Highspeed USB DR Peripheral Controller" depends on FSL_SOC || ARCH_MXC select USB_GADGET_DUALSPEED + select USB_FSL_MPH_DR_OF help Some of Freescale PowerPC processors have a High Speed Dual-Role(DR) USB controller, which supports device mode. diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 2d926ce..6687523 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX support both high speed and full speed devices, or high speed devices only. +config USB_FSL_MPH_DR_OF + bool + depends on PPC_OF + config USB_EHCI_FSL bool "Support for Freescale on-chip EHCI USB controller" depends on USB_EHCI_HCD && FSL_SOC select USB_EHCI_ROOT_HUB_TT + select USB_FSL_MPH_DR_OF ---help--- Variation of ARC USB block used in some Freescale chips. diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index b6315aa..aacbe82 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -33,4 +33,5 @@ obj-$(CONFIG_USB_R8A66597_HCD) += r8a66597-hcd.o obj-$(CONFIG_USB_ISP1760_HCD) += isp1760.o obj-$(CONFIG_USB_HWA_HCD) += hwa-hc.o obj-$(CONFIG_USB_IMX21_HCD) += imx21-hcd.o +obj-$(CONFIG_USB_FSL_MPH_DR_OF) += fsl-mph-dr-of.o diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c new file mode 100644 index 0000000..020a939 --- /dev/null +++ b/drivers/usb/host/fsl-mph-dr-of.c @@ -0,0 +1,189 @@ +/* + * Setup platform devices needed by the Freescale multi-port host + * and/or dual-role USB controller modules based on the description + * in flat device tree. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/fsl_devices.h> +#include <linux/io.h> +#include <linux/of_platform.h> + +struct fsl_usb2_dev_data { + char *dr_mode; /* controller mode */ + char *drivers[3]; /* drivers to instantiate for this mode */ + enum fsl_usb2_operating_modes op_mode; /* operating mode */ +}; + +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = { + { + "host", + { "fsl-ehci", NULL, NULL, }, + FSL_USB2_DR_HOST, + }, + { + "otg", + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", }, + FSL_USB2_DR_OTG, + }, + { + "periferal", + { "fsl-usb2-udc", NULL, NULL, }, + FSL_USB2_DR_DEVICE, + }, +}; + +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np) +{ + const unsigned char *prop; + int i; + + prop = of_get_property(np, "dr_mode", NULL); + if (prop) { + for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) { + if (!strcmp(prop, dr_mode_data[i].dr_mode)) + return &dr_mode_data[i]; + } + } + return &dr_mode_data[0]; /* mode not specified, use host */ +} + +static enum fsl_usb2_phy_modes __devinit determine_usb_phy(const char *phy_type) +{ + if (!phy_type) + return FSL_USB2_PHY_NONE; + if (!strcasecmp(phy_type, "ulpi")) + return FSL_USB2_PHY_ULPI; + if (!strcasecmp(phy_type, "utmi")) + return FSL_USB2_PHY_UTMI; + if (!strcasecmp(phy_type, "utmi_wide")) + return FSL_USB2_PHY_UTMI_WIDE; + if (!strcasecmp(phy_type, "serial")) + return FSL_USB2_PHY_SERIAL; + + return FSL_USB2_PHY_NONE; +} + +struct platform_device * __devinit +fsl_usb2_device_register(struct of_device *ofdev, + struct fsl_usb2_platform_data *pdata, + const char *name, int id) +{ + struct platform_device *pdev; + const struct resource *res = ofdev->resource; + unsigned int num = ofdev->num_resources; + int retval; + + pdev = platform_device_alloc(name, id); + if (!pdev) { + retval = -ENOMEM; + goto error; + } + + pdev->dev.parent = &ofdev->dev; + + pdev->dev.coherent_dma_mask = ofdev->dev.coherent_dma_mask; + pdev->dev.dma_mask = &pdev->archdata.dma_mask; + *pdev->dev.dma_mask = *ofdev->dev.dma_mask; + + retval = platform_device_add_data(pdev, pdata, sizeof(*pdata)); + if (retval) + goto error; + + if (num) { + retval = platform_device_add_resources(pdev, res, num); + if (retval) + goto error; + } + + retval = platform_device_add(pdev); + if (retval) + goto error; + + return pdev; + +error: + platform_device_put(pdev); + return ERR_PTR(retval); +} + +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev, + const struct of_device_id *match) +{ + struct device_node *np = ofdev->dev.of_node; + struct platform_device *usb_dev; + struct fsl_usb2_platform_data data, *pdata; + struct fsl_usb2_dev_data *dev_data; + const unsigned char *prop; + static unsigned int idx; + int i; + + if (!of_device_is_available(np)) + return -ENODEV; + + pdata = match->data; + if (!pdata) { + memset(&data, 0, sizeof(data)); + pdata = &data; + } + + dev_data = get_dr_mode_data(np); + + if (of_device_is_compatible(np, "fsl-usb2-mph")) { + prop = of_get_property(np, "port0", NULL); + if (prop) + pdata->port_enables |= FSL_USB2_PORT0_ENABLED; + + prop = of_get_property(np, "port1", NULL); + if (prop) + pdata->port_enables |= FSL_USB2_PORT1_ENABLED; + + pdata->operating_mode = FSL_USB2_MPH_HOST; + } else { + /* setup mode selected in the device tree */ + pdata->operating_mode = dev_data->op_mode; + } + + prop = of_get_property(np, "phy_type", NULL); + pdata->phy_mode = determine_usb_phy(prop); + + for (i = 0; i < ARRAY_SIZE(dev_data->drivers); i++) { + if (!dev_data->drivers[i]) + continue; + usb_dev = fsl_usb2_device_register(ofdev, pdata, + dev_data->drivers[i], idx); + if (IS_ERR(usb_dev)) { + dev_err(&ofdev->dev, "Can't register usb device\n"); + return PTR_ERR(usb_dev); + } + } + idx++; + return 0; +} + +static const struct of_device_id fsl_usb2_mph_dr_of_match[] = { + { .compatible = "fsl-usb2-mph", }, + { .compatible = "fsl-usb2-dr", }, + {}, +}; + +static struct of_platform_driver fsl_usb2_mph_dr_driver = { + .driver = { + .name = "fsl-usb2-mph-dr", + .owner = THIS_MODULE, + .of_match_table = fsl_usb2_mph_dr_of_match, + }, + .probe = fsl_usb2_mph_dr_of_probe, +}; + +static int __init fsl_usb2_mph_dr_init(void) +{ + return of_register_platform_driver(&fsl_usb2_mph_dr_driver); +} +fs_initcall(fsl_usb2_mph_dr_init);
The driver creates platform devices based on the information from USB nodes in the flat device tree. This is the replacement for old arch fsl_soc usb code removed by the previous patch. It uses usual of-style binding, available EHCI-HCD and UDC drivers can be bound to the created devices. The new of-style driver additionaly instantiates USB OTG platform device, as the appropriate USB OTG driver will be added soon. Signed-off-by: Anatolij Gustschin <agust@denx.de> Cc: Kumar Gala <galak@kernel.crashing.org> Cc: Grant Likely <grant.likely@secretlab.ca> --- drivers/usb/gadget/Kconfig | 1 + drivers/usb/host/Kconfig | 5 + drivers/usb/host/Makefile | 1 + drivers/usb/host/fsl-mph-dr-of.c | 189 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/host/fsl-mph-dr-of.c