Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
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 ag...@denx.de wrote: Hi Grant, On Wed, 28 Jul 2010 02:16:08 -0600 Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin ag...@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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin ag...@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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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 000..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
Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
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. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
Hi Grant, On Wed, 28 Jul 2010 02:16:08 -0600 Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin ag...@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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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.
Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin ag...@denx.de wrote: Hi Grant, On Wed, 28 Jul 2010 02:16:08 -0600 Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin ag...@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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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, + },
Re: [PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
On Wed, Jul 28, 2010 at 12:14:14PM -0600, Grant Likely wrote: On Wed, Jul 28, 2010 at 5:58 AM, Anatolij Gustschin ag...@denx.de wrote: Hi Grant, On Wed, 28 Jul 2010 02:16:08 -0600 Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin ag...@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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/3] USB: add of_platform glue driver for FSL USB DR controller
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 ag...@denx.de Cc: Kumar Gala ga...@kernel.crashing.org Cc: Grant Likely grant.lik...@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 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 000..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