Allow OHCI drivers to override hub control operations
Subsystems (6)
cover Allow OHCI drivers to override hub control operations
Hello,
The PXA27x OHCI implementation doesn't perform automatic control of port power supplies for all ports. While the PPS and LSDA bits of the HcRhPortStatus register are implemented, only a subset of ports have an external power enable pin controlled by the port status register. Other ports need their power supply to be controlled manually.
In order to do so I've implemented manual regulator control in the ohci-pxa27x driver. This requires overriding the default behaviour of the CLEAR_FEATURE and SET_FEATURE requests for USB_PORT_FEAT_POWER with a custom hub control operation. In turn this requires calling the currently static ohci_hub_control function from the ohci-pxa27x driver.
The ohci-s3c2410 driver already implements a similar feature, and accesses the ohci_hub_control function by saving the struct hc_driver hub_control value to a global variable right after calling ohci_init_driver. This is a bit of a hack, and as I need to call that function as well I've decided to export it instead.
Another option would have been to handle the regulators directly inside the ohci-hub implementation, but I'm not sure whether it's worth it yet. Comments (or acks :-)) will be appreciated.
Please not that I haven't been able to test the third patch due to lack of hardware. I've however tested a similar implementation on an out of tree PXA270 board.
Laurent Pinchart (3):
USB: OHCI: Export the ohci_hub_control function
USB: ohci-pxa27x: Add support for external vbus regulators
ARM: pxa: zeus: Replace OHCI init/exit functions with a regulator
arch/arm/mach-pxa/zeus.c | 89 ++++++++++++++++++++++------------------- drivers/usb/host/ohci-hub.c | 4 +- drivers/usb/host/ohci-pxa27x.c | 67 +++++++++++++++++++++++++++++++ drivers/usb/host/ohci-s3c2410.c | 7 +--- drivers/usb/host/ohci.h | 2 + 5 files changed, 121 insertions(+), 48 deletions(-)
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/3 USB: OHCI: Export the ohci_hub_control function
Platform drivers sometimes need to perform specific handling of hub control requests. Make this possible by exporting the ohci_hub_control() function which can then be called from a custom hub control handler in the default case.
--- drivers/usb/host/ohci-hub.c | 4 ++-- drivers/usb/host/ohci-s3c2410.c | 7 ++----- drivers/usb/host/ohci.h | 2 ++ 3 files changed, 6 insertions(+), 7 deletions(-)
drivers/usb/host/ohci-hub.c +2 -2
@@ -646,7 +646,7 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)return 0 ; } -static int ohci_hub_control ( +int ohci_hub_control ( struct usb_hcd *hcd, u16 typeReq, u16 wValue,
@@ -772,4 +772,4 @@ error: }return retval; } - +EXPORT_SYMBOL_GPL (ohci_hub_control);
drivers/usb/host/ohci-s3c2410.c +2 -5
@@ -45,8 +45,6 @@ static struct clk *usb_clk;/* forward definitions */ -static int (*orig_ohci_hub_control)(struct usb_hcd *hcd,u16 typeReq, -u16 wValue,u16 wIndex,char *buf,u16 wLength);static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd,char *buf);static void s3c2410_hcd_oc (struct s3c2410_hcd_info *info,int port_oc);
@@ -181,7 +179,7 @@ static int ohci_s3c2410_hub_control(* process the request straight away and exit */ if (info ==NULL ) { - ret =orig_ohci_hub_control (hcd, typeReq, wValue, + ret =ohci_hub_control (hcd, typeReq, wValue, wIndex, buf, wLength); goto out; }
@@ -231,7 +229,7 @@ static int ohci_s3c2410_hub_control(break ; } - ret =orig_ohci_hub_control (hcd, typeReq, wValue, wIndex, buf, wLength); + ret =ohci_hub_control (hcd, typeReq, wValue, wIndex, buf, wLength);if (ret) goto out;
@@ -489,7 +487,6 @@ static int __init ohci_s3c2410_init(void)* override these functions by making it too easy. */ - orig_ohci_hub_control = ohci_s3c2410_hc_driver.hub_control; orig_ohci_hub_status_data = ohci_s3c2410_hc_driver.hub_status_data; ohci_s3c2410_hc_driver.hub_status_data = ohci_s3c2410_hub_status_data;
drivers/usb/host/ohci.h +2 -0
@@ -727,3 +727,5 @@ extern int ohci_setup(struct usb_hcd *hcd);extern int ohci_suspend (struct usb_hcd *hcd,bool do_wakeup);extern int ohci_resume (struct usb_hcd *hcd,bool hibernated);#endif +extern int ohci_hub_control (struct usb_hcd *hcd,u16 typeReq,u16 wValue, +u16 wIndex,char *buf,u16 wLength);
Signature · 1 line
1.8.3.2
PATCH 2/3 USB: ohci-pxa27x: Add support for external vbus regulators
In reply to Laurent PinchartOverride the hub control operation to enable and disable external regulators for the ports vbus power supply in response to clear/set USB_PORT_FEAT_POWER requests.
--- drivers/usb/host/ohci-pxa27x.c | 67 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)
drivers/usb/host/ohci-pxa27x.c +67 -0
@@ -30,6 +30,7 @@#include <linux/platform_data/usb-ohci-pxa27x.h> #include <linux/platform_data/usb-pxa3xx-ulpi.h> #include <linux/platform_device.h> +#include <linux/regulator/consumer.h> #include <linux/signal.h> #include <linux/usb.h> #include <linux/usb/hcd.h>
@@ -120,6 +121,8 @@ static struct hc_driver __read_mostly ohci_pxa27x_hc_driver;struct pxa27x_ohci {struct clk *clk;void __iomem *mmio_base; +struct regulator *vbus[3 ]; +bool vbus_enabled[3 ]; };#define to_pxa27x_ohci (hcd) (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->priv)
@@ -166,6 +169,52 @@ static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int mode)return 0 ; } +static int pxa27x_ohci_set_vbus_power (struct pxa27x_ohci *pxa_ohci, +unsigned int port,bool enable) +{ +struct regulator *vbus = pxa_ohci->vbus[port]; +int ret =0 ; + +if (IS_ERR_OR_NULL (vbus)) +return 0 ; + +if (enable && !pxa_ohci->vbus_enabled[port]) + ret =regulator_enable (vbus); +else if (!enable && pxa_ohci->vbus_enabled[port]) + ret =regulator_disable (vbus); + +if (ret <0 ) +return ret; + + pxa_ohci->vbus_enabled[port] = enable; + +return 0 ; +} + +static int pxa27x_ohci_hub_control (struct usb_hcd *hcd,u16 typeReq,u16 wValue, +u16 wIndex,char *buf,u16 wLength) +{ +struct pxa27x_ohci *pxa_ohci =to_pxa27x_ohci (hcd); +int ret; + +switch (typeReq) { +case SetPortFeature: +case ClearPortFeature: +if (!wIndex || wIndex >3 ) +return -EPIPE ; + +if (wValue !=USB_PORT_FEAT_POWER ) +break ; + + ret =pxa27x_ohci_set_vbus_power (pxa_ohci, wIndex -1 , + typeReq == SetPortFeature); +if (ret) +return ret; +break ; + } + +return ohci_hub_control (hcd, typeReq, wValue, wIndex, buf, wLength); +}/*-------------------------------------------------------------------------*/ static inline void pxa27x_setup_hc(struct pxa27x_ohci *pxa_ohci,
@@ -372,6 +421,7 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_devicestruct ohci_hcd *ohci;struct resource *r;struct clk *usb_clk; +unsigned int i; retval =ohci_pxa_of_init (pdev);if (retval)
@@ -417,6 +467,16 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device pxa_ohci->clk = usb_clk; pxa_ohci->mmio_base = (void __iomem *)hcd->regs; +for (i =0 ; i <3 ; ++i) { +char name[6 ]; + +if (!(inf->flags & (ENABLE_PORT1 << i))) +continue ; + +sprintf (name,"vbus%u" , i +1 ); + pxa_ohci->vbus[i] =devm_regulator_get (&pdev->dev, name); + } + retval =pxa27x_start_hc (pxa_ohci, &pdev->dev);if (retval <0 ) {pr_debug ("pxa27x_start_hc failed" );
@@ -462,6 +522,10 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_devicevoid usb_hcd_pxa27x_remove (struct usb_hcd *hcd,struct platform_device *pdev) {struct pxa27x_ohci *pxa_ohci =to_pxa27x_ohci (hcd); +unsigned int i; + +for (i =0 ; i <3 ; ++i) +pxa27x_ohci_set_vbus_power (pxa_ohci, i, false);usb_remove_hcd (hcd);pxa27x_stop_hc (pxa_ohci, &pdev->dev);
@@ -563,7 +627,10 @@ static int __init ohci_pxa27x_init(void)return -ENODEV ;pr_info ("%s: " DRIVER_DESC "\n" , hcd_name); +ohci_init_driver (&ohci_pxa27x_hc_driver, &pxa27x_overrides); + ohci_pxa27x_hc_driver.hub_control = pxa27x_ohci_hub_control; +return platform_driver_register (&ohci_hcd_pxa27x_driver); }module_init (ohci_pxa27x_init);
Signature · 1 line
1.8.3.2
PATCH 3/3 ARM: pxa: zeus: Replace OHCI init/exit functions with a regulator
In reply to Laurent Pinchart--- arch/arm/mach-pxa/zeus.c | 89 ++++++++++++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 41 deletions(-)
arch/arm/mach-pxa/zeus.c +48 -41
@@ -413,7 +413,7 @@ static struct fixed_voltage_config can_regulator_pdata = {static struct platform_device can_regulator_device = { .name ="reg-fixed-volage" , - .id =-1 , + .id =0 , .dev = { .platform_data = &can_regulator_pdata, },
@@ -510,18 +510,6 @@ struct platform_device zeus_max6369_device = { .num_resources = 1, }; -static struct platform_device *zeus_devices[]__initdata = { - &zeus_serial_device, - &zeus_mtd_devices[0 ], - &zeus_dm9k0_device, - &zeus_dm9k1_device, - &zeus_sram_device, - &zeus_leds_device, - &zeus_pcmcia_device, - &zeus_max6369_device, - &can_regulator_device, -}; -/* AC'97 */ static pxa2xx_audio_ops_t zeus_ac97_info = { .reset_gpio =95 ,
@@ -532,44 +520,50 @@ static pxa2xx_audio_ops_t zeus_ac97_info = {* USB host */ -static int zeus_ohci_init (struct device *dev) -{ -int err; - -/* Switch on port 2. */ -if ((err =gpio_request (ZEUS_USB2_PWREN_GPIO ,"USB2_PWREN" ))) { -dev_err (dev,"Can't request USB2_PWREN\n" ); -return err; - } - -if ((err =gpio_direction_output (ZEUS_USB2_PWREN_GPIO ,1 ))) { -gpio_free (ZEUS_USB2_PWREN_GPIO ); -dev_err (dev,"Can't enable USB2_PWREN\n" ); -return err; - } +static struct regulator_consumer_supply zeus_ohci_regulator_supplies[] = { +REGULATOR_SUPPLY ("vbus2" ,"pxa27x-ohci" ), +}; -/* Port 2 is shared between host and client interface. */ -UP2OCR =UP2OCR_HXOE |UP2OCR_HXS |UP2OCR_DMPDE |UP2OCR_DPPDE ; +static struct regulator_init_data zeus_ohci_regulator_data = { + .constraints = { + .valid_ops_mask =REGULATOR_CHANGE_STATUS , + }, + .num_consumer_supplies =ARRAY_SIZE (zeus_ohci_regulator_supplies), + .consumer_supplies = zeus_ohci_regulator_supplies, +}; -return 0 ; -} +static struct fixed_voltage_config zeus_ohci_regulator_config = { + .supply_name ="vbus2" , + .microvolts =5000000 ,/* 5.0V */ + .gpio =ZEUS_USB2_PWREN_GPIO , + .enable_high =1 , + .startup_delay =0 , + .init_data = &zeus_ohci_regulator_data, +}; -static void zeus_ohci_exit (struct device *dev) -{ -/* Power-off port 2 */ -gpio_direction_output (ZEUS_USB2_PWREN_GPIO ,0 ); -gpio_free (ZEUS_USB2_PWREN_GPIO ); -} +static struct platform_device zeus_ohci_regulator_device = { + .name ="reg-fixed-voltage" , + .id =1 , + .dev = { + .platform_data = &zeus_ohci_regulator_config, + }, +};static struct pxaohci_platform_data zeus_ohci_platform_data = { .port_mode =PMM_NPS_MODE ,/* Clear Power Control Polarity Low and set Power Sense * Polarity Low. Supply power to USB ports. */ .flags =ENABLE_PORT_ALL |POWER_SENSE_LOW , - .init = zeus_ohci_init, - .exit = zeus_ohci_exit, }; +static void zeus_register_ohci (void ) +{ +/* Port 2 is shared between host and client interface. */ +UP2OCR =UP2OCR_HXOE |UP2OCR_HXS |UP2OCR_DMPDE |UP2OCR_DPPDE ; + +pxa_set_ohci_info (&zeus_ohci_platform_data); +} +/* * Flat Panel */
@@ -677,6 +671,19 @@ static struct pxa2xx_udc_mach_info zeus_udc_info = { .udc_command = zeus_udc_command, }; +static struct platform_device *zeus_devices[]__initdata = { + &zeus_serial_device, + &zeus_mtd_devices[0 ], + &zeus_dm9k0_device, + &zeus_dm9k1_device, + &zeus_sram_device, + &zeus_leds_device, + &zeus_pcmcia_device, + &zeus_max6369_device, + &can_regulator_device, + &zeus_ohci_regulator_device, +}; +#ifdef CONFIG_PM static void zeus_power_off (void ) {
@@ -847,7 +854,7 @@ static void __init zeus_init(void)platform_add_devices (zeus_devices,ARRAY_SIZE (zeus_devices)); -pxa_set_ohci_info (&zeus_ohci_platform_data); +zeus_register_ohci ();if (zeus_setup_fb_gpios ())pr_err ("Failed to setup fb gpios\n" );
Signature · 1 line
1.8.3.2
Alan Stern
maintainercover Allow OHCI drivers to override hub control operations
In reply to Laurent PinchartQuoted 14 lines · Laurent Pinchart
Hello,
The PXA27x OHCI implementation doesn't perform automatic control of port power supplies for all ports. While the PPS and LSDA bits of the HcRhPortStatus register are implemented, only a subset of ports have an external power enable pin controlled by the port status register. Other ports need their power supply to be controlled manually.
In order to do so I've implemented manual regulator control in the ohci-pxa27x driver. This requires overriding the default behaviour of the
CLEAR_FEATUREandSET_FEATURErequests forUSB_PORT_FEAT_POWERwith a custom hub control operation. In turn this requires calling the currently staticohci_hub_controlfunction from the ohci-pxa27x driver.
Quoted 5 lines
The ohci-s3c2410 driver already implements a similar feature, and accesses the
ohci_hub_controlfunction by saving the structhc_driverhub_controlvalue to a global variable right after callingohci_init_driver. This is a bit of a hack, and as I need to call that function as well I've decided to export it instead.
It only seems like a hack if you don't think about it the right way. :-)
The intention was to imitate an object-oriented style, by allowing a child class to override member functions in the parent class. The vtable mechanism in C++, for example, does essentially the same thing as ohci-s3c2410 (except that C++ doesn't save the original function pointer in a global variable; instead it reads the function pointer from the parent's vtable as needed -- ohci-s3c2410 can't do that because ohci_hc_driver isn't exported).
I'm open to the idea of exporting the hub functions. In the end, it doesn't make all that much difference (it would save a little object code).
Quoted 13 lines
Another option would have been to handle the regulators directly inside the ohci-hub implementation, but I'm not sure whether it's worth it yet. Comments (or acks :-)) will be appreciated.
Please not that I haven't been able to test the third patch due to lack of hardware. I've however tested a similar implementation on an out of tree PXA270 board.
Laurent Pinchart (3):
USB: OHCI: Export theohci_hub_controlfunction
USB: ohci-pxa27x: Add support for external vbus regulators
ARM: pxa: zeus: Replace OHCI init/exit functions with a regulator
Quoted 6 lines
arch/arm/mach-pxa/zeus.c | 89 ++++++++++++++++++++++------------------- drivers/usb/host/ohci-hub.c | 4 +- drivers/usb/host/ohci-pxa27x.c | 67 +++++++++++++++++++++++++++++++ drivers/usb/host/ohci-s3c2410.c | 7 +--- drivers/usb/host/ohci.h | 2 + 5 files changed, 121 insertions(+), 48 deletions(-)
You missed ohci-at91.c. And ehci-tegra.c plays similar games. I'd prefer to do the same thing for both ehci-hcd and ohci-hcd.
Alan Stern
cover Allow OHCI drivers to override hub control operations
Hi Alan,
Thank you for your comments.
Quoted 21 lines · On Monday 14 April 2014 10:35:43 Alan Stern wrote:
Quoted 14 lines · Laurent Pinchart
Hello,
The PXA27x OHCI implementation doesn't perform automatic control of port power supplies for all ports. While the PPS and LSDA bits of the HcRhPortStatus register are implemented, only a subset of ports have an external power enable pin controlled by the port status register. Other ports need their power supply to be controlled manually.
In order to do so I've implemented manual regulator control in the ohci-pxa27x driver. This requires overriding the default behaviour of the
CLEAR_FEATUREandSET_FEATURErequests forUSB_PORT_FEAT_POWERwith a custom hub control operation. In turn this requires calling the currently staticohci_hub_controlfunction from the ohci-pxa27x driver.Quoted 5 lines
The ohci-s3c2410 driver already implements a similar feature, and accesses the
ohci_hub_controlfunction by saving the structhc_driverhub_controlvalue to a global variable right after callingohci_init_driver. This is a bit of a hack, and as I need to call that function as well I've decided to export it instead.
Quoted 8 lines
It only seems like a hack if you don't think about it the right way. :-)
The intention was to imitate an object-oriented style, by allowing a child class to override member functions in the parent class. The vtable mechanism in C++, for example, does essentially the same thing as ohci-s3c2410 (except that C++ doesn't save the original function pointer in a global variable; instead it reads the function pointer from the parent's vtable as needed -- ohci-s3c2410 can't do that because
ohci_hc_driverisn't exported).
gcc 4.9 has been released with a new speculative devirtualization optimization pass. As we don't use C++ we can't make use of that feature, so we need to perform the optimization manually, hence this change :-)
Quoted 2 lines
I'm open to the idea of exporting the hub functions. In the end, it doesn't make all that much difference (it would save a little object code).
I would have agreed to keep the code as it is today if you had thought that exporting the hub functions was a really bad idea, but as you're open to it, and as it removes a bit of code without much of a drawback, I think it makes sense to perform that optimization.
Quoted 20 lines
Quoted 13 lines
Another option would have been to handle the regulators directly inside the ohci-hub implementation, but I'm not sure whether it's worth it yet. Comments (or acks :-)) will be appreciated.
Please not that I haven't been able to test the third patch due to lack of hardware. I've however tested a similar implementation on an out of tree PXA270 board.
Laurent Pinchart (3):
USB: OHCI: Export theohci_hub_controlfunction
USB: ohci-pxa27x: Add support for external vbus regulators
ARM: pxa: zeus: Replace OHCI init/exit functions with a regulatorQuoted 6 lines
arch/arm/mach-pxa/zeus.c | 89 ++++++++++++++++++------------------ drivers/usb/host/ohci-hub.c | 4 +- drivers/usb/host/ohci-pxa27x.c | 67 +++++++++++++++++++++++++++++++ drivers/usb/host/ohci-s3c2410.c | 7 +--- drivers/usb/host/ohci.h | 2 + 5 files changed, 121 insertions(+), 48 deletions(-)
Quoted 2 lines
You missed
ohci-at91.c. Andehci-tegra.cplays similar games. I'd prefer to do the same thing for both ehci-hcd and ohci-hcd.
OK. I'll work on a v2 of patch 1/3 that modifies ohci-at91.c as well, and I'll add a new patch to perform the same change for ehci-hub.c and ehci-tegra.c.
By the way, as a second step, do you think it would make sense to handle the vbus regulators directly in ohci_hub_control() ?
Signature · 3 lines
Regards, Laurent Pinchart
Alan Stern
maintainercover Allow OHCI drivers to override hub control operations
Quoted 7 lines · Laurent Pinchart
Quoted 2 lines
I'm open to the idea of exporting the hub functions. In the end, it doesn't make all that much difference (it would save a little object code).
I would have agreed to keep the code as it is today if you had thought that exporting the hub functions was a really bad idea, but as you're open to it, and as it removes a bit of code without much of a drawback, I think it makes sense to perform that optimization.
Agreed. We aren't really _encouraging_ people to do this, because it still takes some manual work to override the default operation.
Quoted 2 lines
By the way, as a second step, do you think it would make sense to handle the vbus regulators directly in
ohci_hub_control()?
I don't see any good way of doing this generically. Since it is currently needed only for PXA27x, we don't know what other architectures will require in the future.
Alan Stern
cover Allow OHCI drivers to override hub control operations
Hi Alan,
Quoted 13 lines · On Tuesday 15 April 2014 14:05:14 Alan Stern wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 3 lines
I'm open to the idea of exporting the hub functions. In the end, it doesn't make all that much difference (it would save a little object code).
I would have agreed to keep the code as it is today if you had thought that exporting the hub functions was a really bad idea, but as you're open to it, and as it removes a bit of code without much of a drawback, I think it makes sense to perform that optimization.
Agreed. We aren't really _encouraging_ people to do this, because it still takes some manual work to override the default operation.
Quoted 6 lines
Quoted 2 lines
By the way, as a second step, do you think it would make sense to handle the vbus regulators directly in
ohci_hub_control()?I don't see any good way of doing this generically. Since it is currently needed only for PXA27x, we don't know what other architectures will require in the future.
Fine with me, let's revisit this later if needed then.
Signature · 3 lines
Regards, Laurent Pinchart