Re: [PATCH v5 3/7] phy: Add set_mode callback

[Kishon Vijay Abraham I]

Hi,

On Tuesday 10 May 2016 05:09 AM, David Lechner wrote:
> The initial use for this is for PHYs that have a mode related to USB OTG.
> There are several SoCs (e.g. TI OMAP and DA8xx) that have a mode setting
> in the USB PHY to override OTG VBUS and ID signals.
> 
> Of course, the enum can be expaned in the future to include modes for
> other types of PHYs as well.
> 
> Suggested-by: Kishon Vijay Abraham I 
> Signed-off-by: David Lechner 

This patch is required for both phy driver and the usb driver. I can create a
immutable branch with only this patch and both me and Bin Liu can merge it in
our next branch.

Thanks
Kishon

> ---
>  drivers/phy/phy-core.c  | 15 +++
>  include/linux/phy/phy.h | 17 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index e7e574d..fe0344c 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -342,6 +342,21 @@ int phy_power_off(struct phy *phy)
>  }
>  EXPORT_SYMBOL_GPL(phy_power_off);
>  
> +int phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> + int ret;
> +
> + if (!phy || !phy->ops->set_mode)
> + return 0;
> +
> + mutex_lock(>mutex);
> + ret = phy->ops->set_mode(phy, mode);
> + mutex_unlock(>mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_set_mode);
> +
>  /**
>   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>   * @np: device_node for which to get the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 8cf05e3..4248ade 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -22,12 +22,20 @@
>  
>  struct phy;
>  
> +enum phy_mode {
> + PHY_MODE_INVALID,
> + PHY_MODE_USB_HOST,
> + PHY_MODE_USB_DEVICE,
> + PHY_MODE_USB_OTG,
> +};
> +
>  /**
>   * struct phy_ops - set of function pointers for performing phy operations
>   * @init: operation to be performed for initializing phy
>   * @exit: operation to be performed while exiting
>   * @power_on: powering on the phy
>   * @power_off: powering off the phy
> + * @set_mode: set the mode of the phy
>   * @owner: the module owner containing the ops
>   */
>  struct phy_ops {
> @@ -35,6 +43,7 @@ struct phy_ops {
>   int (*exit)(struct phy *phy);
>   int (*power_on)(struct phy *phy);
>   int (*power_off)(struct phy *phy);
> + int (*set_mode)(struct phy *phy, enum phy_mode mode);
>   struct module *owner;
>  };
>  
> @@ -119,6 +128,7 @@ int phy_init(struct phy *phy);
>  int phy_exit(struct phy *phy);
>  int phy_power_on(struct phy *phy);
>  int phy_power_off(struct phy *phy);
> +int phy_set_mode(struct phy *phy, enum phy_mode mode);
>  static inline int phy_get_bus_width(struct phy *phy)
>  {
>   return phy->attrs.bus_width;
> @@ -224,6 +234,13 @@ static inline int phy_power_off(struct phy *phy)
>   return -ENOSYS;
>  }
>  
> +static inline int phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> + if (!phy)
> + return 0;
> + return -ENOSYS;
> +}
> +
>  static inline int phy_get_bus_width(struct phy *phy)
>  {
>   return -ENOSYS;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

stuck after usb_kill_urb

[Saurin G. Suthar]

Hi,

I am using linux kernel 4.1.15Here is the "lsusb -tv² output (Just shown
related devices)
# lsusb -tv
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=ohci-pci/3p, 12M
|__ Port 1: Dev 2, If 0, Class=Communications, Driver=cdc_acm, 12M
|__ Port 1: Dev 2, If 1, Class=CDC Data, Driver=cdc_acm, 12M
|__ Port 3: Dev 3, If 0, Class=Vendor Specific Class, Driver=ftdi_sio,
12Mw

We have /dev/ttyUSB0 and /dev/ttyACM0 devices, system works for few
minutes when we use /dev/ttyACM0 device to communicate, but after running
couple of minutes  /dev/ttyACM0 device doesn't respond and stuck
infinitly, by looking at the process stack trace it shows following. what
could be the reason? I also tried to backport drivers/usb/core/urb.c,
drivers/usb/core/hcd.c, drivers/usb/class/cdc-acm.c from v4.7-rc4 to
v4.1.15, still getting the same issue.

# cat /proc/45194/stack
[] usb_kill_urb+0x85/0xc0
[] acm_port_shutdown+0xb0/0xd0
[] tty_port_shutdown+0x74/0xa0
[] tty_port_close+0x21/0x50
[] acm_tty_close+0x1d/0x20
[] tty_release+0x12b/0x5e0
[] __fput+0x97/0x1d0
[] fput+0x9/0x10
[] task_work_run+0xb7/0xf0
[] do_notify_resume+0x65/0x70
[] int_signal+0x12/0x17


# dmesg
[  845.334978] tty ttyACM0: acm_tty_install
[  845.334986] tty ttyACM0: acm_tty_open
[  845.334988] cdc_acm 4-1:1.0: acm_port_activate
[  845.334997] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 0
[  845.335000] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 1
[  845.335002] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 2
[  845.335004] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 3
[  845.335006] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 4
[  845.335009] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 5
[  845.335010] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 6
[  845.335012] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 7
[  845.335014] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 8
[  845.335017] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 9
[  845.335019] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 10
[  845.335021] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 11
[  845.335023] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 12
[  845.335026] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 13
[  845.335028] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 14
[  845.335030] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 15
[  845.336100] cdc_acm 4-1:1.0: acm_ctrl_msg - rq 0x22, val 0x3, len 0x0,
result 0
[  845.337117] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 0, len 1
[  845.337152] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 0
[  845.337256] cdc_acm 4-1:1.1: acm_tty_write - count 3
[  845.337259] cdc_acm 4-1:1.1: acm_tty_write - write 3
[  845.338100] cdc_acm 4-1:1.1: acm_softint
[  845.339089] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 1, len 12
[  845.339094] cdc_acm 4-1:1.1: acm_submit_read_urb - urb 1
[  845.339126] cdc_acm 4-1:1.0: acm_tty_close
[  845.340103] cdc_acm 4-1:1.0: acm_ctrl_msg - rq 0x22, val 0x0, len 0x0,
result 0
[  845.340105] cdc_acm 4-1:1.0: acm_port_shutdown
[  845.341176] cdc_acm 4-1:1.0: acm_ctrl_irq - urb shutting down with
status: -2
[  845.342160] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 0, len 0
[  845.342225] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.342313] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 1, len 0
[  845.342377] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.342465] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 2, len 0
[  845.342530] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.342618] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 3, len 0
[  845.342622] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.343095] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 4, len 0
[  845.343098] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.344122] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 5, len 0
[  845.344137] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.345104] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 6, len 0
[  845.345106] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.346110] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 7, len 0
[  845.346114] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.347099] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 8, len 0
[  845.347102] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.348100] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 9, len 0
[  845.348103] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.349103] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 10, len 0
[  845.349106] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.350103] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 11, len 0
[  845.350106] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb
status: -2
[  845.351107] cdc_acm 4-1:1.1: acm_read_bulk_callback - urb 12, len 0
[  845.351110] cdc_acm 4-1:1.1: acm_read_bulk_callback - non-zero urb

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 05:47:47PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> >> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with 
> >> >> >> >> >>> OTG core.
> >> >> >> >> >> 
> >> >> >> >> >> do you really know of any platform which has a separate OTG 
> >> >> >> >> >> controller?
> >> >> >> >> >> 
> >> >> >> >> >
> >> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks 
> >> >> >> >> > for OTG, host
> >> >> >> >> > and gadget.
> >> >> >> >> >
> >> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> >> >> 
> >> >> >> >> that's not an OTG controller, it's just a mux. No different than 
> >> >> >> >> Intel's
> >> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> >> >> 
> >> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into 
> >> >> >> >> play. They
> >> >> >> >> are two competing standards and, apparently, type-C is winning 
> >> >> >> >> when it
> >> >> >> >> comes to role-swapping.
> >> >> >> >> 
> >> >> >> >
> >> >> >> > In fact, OTG is mis-used by people. Currently, if the port is 
> >> >> >> > dual-role,
> >> >> >> > It will be considered as an OTG port.
> >> >> >> 
> >> >> >> That's because "dual-role" is a non-standard OTG. Seen as people 
> >> >> >> really
> >> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally 
> >> >> >> referring
> >> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >> >> >
> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> >> > "OTG" in Linux, right?
> >> >> 
> >> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> >> figure out how you can deliver what your marketting guys are asking of
> >> >> you.
> >> >> 
> >> >
> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> >> > test. The internal bsp has passed PET test, and formal compliance test
> >> > is on the way (should pass too). 
> >> >
> >> > The dual-role and OTG compliance use the same zImage, but different
> >> > dtb.
> >> 
> >> okay, that's good to know. Now, the question really is: considering we
> >> only have one user for this generic OTG FSM layer, do we really need to
> >> make it generic at all? I mean, just look at how invasive a change that
> >> is.
> >
> > If the chipidea is the only user for this roger's framework, I don't
> > think it is necessary. In fact, Roger introduces this framework, and
> > the first user is dwc3, we think it can be used for others. Let's
> 
> Right, we need to look at the history of dwc3 to figure out why the
> conclusion that dwc3 needs this was made.
> 
> Roger started working on this framework when Power on Reset section of
> databook had some details which weren't always clear and, for safety, we
> always had reset asserted for a really long time. It was so long (about
> 400 ms) that resetting dwc3 for each role swap was just too much.
> 
> Coupled with that, the OTG chapter wasn't very clear either on
> expections from Host and Peripheral side initialization in OTG/DRD
> systems.
> 
> More recent version of dwc3 databook have a much better description of
> how and which reset bits _must_ be asserted and which shouldn't be
> touched unless it's for debugging purposes. When I implemented that, our
> ->probe() went from 400ms down to about 50us.
> 
> Coupled with that, the OTG chapter also became a lot clearer to the
> point that it states you just don't initialize anything other than the
> OTG block, and just wait for OTG interrupt to do whatever it is you need
> to do.
> 
> This meant that we could actually afford to do full reinitialization of
> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
> roles properly.
> 
> (The reason for needing soft-reset during role swap is kinda long. But
> in summary dwc3 shadows register writes to both host and peripheral
> sides)
> 
> > just discuss if it is necessary for dual-role switch.
> 
> fair. However, if we have a single user we don't have a Generic
> layer. There's not enough variance to come up with truly generic
> architecture for this.
> 
> -- 

I have put some points in my last reply [1], I summery it here to
see if a generic framework is deserved or not?

1. If there are some parts we can use during the role switch
- The common start/stop host and peripheral operation
eg, when switch from host to peripheral, all drivers can use
usb_remove_hcd to finish it.
- A common workqueue to handle vbus and id event
- sysfs for role switch

2. Does a mux driver can do it well? Yoshihiro, here we need your
point. The main point is if we need to call USB API to change
roles (eg, usb_remove_hcd) during the role switch, thanks.


[1] http://www.spinics.net/lists/linux-usb/msg142974.html
-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More 

[PATCH 3/3] dt-bindings: Document the STM32 USB OTG DWC2 core binding

[Bruno Herrera]

Signed-off-by: Bruno Herrera 
---
 Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt 
b/Documentation/devicetree/bindings/usb/dwc2.txt
index 20a68bf..79e5370 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -11,6 +11,7 @@ Required properties:
   - "lantiq,arx100-usb": The DWC2 USB controller instance in Lantiq ARX SoCs;
   - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
+  - st,stm32-fsotg: The DWC2 USB controller instance in STM32F4 SoCs in FS 
mode;
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt
 - clocks: clock provider specifier
-- 
2.7.4 (Apple Git-66)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] ARM: dts: STM32 Add USB FS host mode support

[Bruno Herrera]

Signed-off-by: Bruno Herrera 
---
 arch/arm/boot/dts/stm32f429-disco.dts | 30 ++
 arch/arm/boot/dts/stm32f429.dtsi  | 33 -
 arch/arm/boot/dts/stm32f469-disco.dts | 30 ++
 3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32f429-disco.dts 
b/arch/arm/boot/dts/stm32f429-disco.dts
index 0140807..8e0114b 100644
--- a/arch/arm/boot/dts/stm32f429-disco.dts
+++ b/arch/arm/boot/dts/stm32f429-disco.dts
@@ -75,6 +75,16 @@
linux,default-trigger = "heartbeat";
};
};
+
+   /* This turns on vbus for otg for host mode (dwc2) */
+   vcc5v_otg: vcc5v-otg-regulator {
+   compatible = "regulator-fixed";
+   gpio = < 4 0>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_pwren_h>;
+   regulator-name = "vcc5_host1";
+   regulator-always-on;
+   };
 };
 
 _hse {
@@ -86,3 +96,23 @@
pinctrl-names = "default";
status = "okay";
 };
+
+_hs {
+   compatible = "st,stm32-fsotg", "snps,dwc2";
+   dr_mode = "host";
+   pinctrl-0 = <_fs_pins_b>;
+   pinctrl-names = "default";
+   status = "okay";
+};
+
+ {
+   usb-host {
+   usbotg_pwren_h: usbotg-pwren-h {
+   pins {
+   pinmux = ;
+   bias-disable;
+   drive-push-pull;
+   };
+   };
+   };
+};
diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index 35df462..cb33aa2 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -176,7 +176,7 @@
reg = <0x40013800 0x400>;
};
 
-   pin-controller {
+   pinctrl: pin-controller {
#address-cells = <1>;
#size-cells = <1>;
compatible = "st,stm32f429-pinctrl";
@@ -284,6 +284,28 @@
};
};
 
+   usbotg_fs_pins_a: usbotg_fs@0 {
+   pins {
+   pinmux = 
,
+
,
+
;
+   bias-disable;
+   drive-push-pull;
+   slew-rate = <2>;
+   };
+   };
+
+   usbotg_fs_pins_b: usbotg_fs@1 {
+   pins {
+   pinmux = 
,
+
,
+
;
+   bias-disable;
+   drive-push-pull;
+   slew-rate = <2>;
+   };
+   };
+
usbotg_hs_pins_a: usbotg_hs@0 {
pins {
pinmux = 
,
@@ -388,6 +410,15 @@
status = "disabled";
};
 
+   usbotg_fs: usb@5000 {
+   compatible = "st,stm32-fsotg", "snps,dwc2";
+   reg = <0x5000 0x4>;
+   interrupts = <67>;
+   clocks = < 0 39>;
+   clock-names = "otg";
+   status = "disabled";
+   };
+
rng: rng@50060800 {
compatible = "st,stm32-rng";
reg = <0x50060800 0x400>;
diff --git a/arch/arm/boot/dts/stm32f469-disco.dts 
b/arch/arm/boot/dts/stm32f469-disco.dts
index 83ee90d..46e5279 100644
--- a/arch/arm/boot/dts/stm32f469-disco.dts
+++ b/arch/arm/boot/dts/stm32f469-disco.dts
@@ -64,6 +64,17 @@
aliases {
serial0 = 
};
+
+   /* This turns on vbus for otg for host mode (dwc2) */
+   vcc5v_otg: vcc5v-otg-regulator {
+   compatible = "regulator-fixed";
+   enable-active-high;
+   gpio = < 2 0>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_pwren_h>;
+   regulator-name = "vcc5_host1";
+   regulator-always-on;
+   };
 };
 
 _hse {
@@ -73,3 +84,22 @@
  {
status = "okay";
 };
+
+_fs {
+   dr_mode = "host";
+   pinctrl-0 = <_fs_pins_a>;
+   pinctrl-names = "default";
+   status = "okay";
+};
+
+ {
+   usb-host {
+   usbotg_pwren_h: usbotg-pwren-h {
+   pins {
+   pinmux = ;
+   bias-disable;
+   

[PATCH 1/3] usb: dwc2: Add support for STM32F429/439/469 USB OTG in FS mode with internal PHY

[Bruno Herrera]

Signed-off-by: Bruno Herrera 
---
 drivers/usb/dwc2/core.c | 18 ++
 drivers/usb/dwc2/core.h |  5 +
 drivers/usb/dwc2/hcd.c  | 12 +++-
 drivers/usb/dwc2/hw.h   |  2 ++
 drivers/usb/dwc2/platform.c | 37 +
 5 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 4135a5f..83fbed6 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1276,6 +1276,23 @@ static void dwc2_set_param_hibernation(struct dwc2_hsotg 
*hsotg,
hsotg->core_params->hibernation = val;
 }
 
+static void dwc2_set_param_stm32_powerdown(struct dwc2_hsotg *hsotg,
+   int val)
+{
+   if (DWC2_OUT_OF_BOUNDS(val, 0, 1)) {
+   if (val >= 0) {
+   dev_err(hsotg->dev,
+   "'%d' invalid for parameter power down\n",
+   val);
+   dev_err(hsotg->dev, "power down must be 0 or 1\n");
+   }
+   val = 0;
+   dev_dbg(hsotg->dev, "Setting power down to %d\n", val);
+   }
+
+   hsotg->core_params->stm32_powerdown = val;
+}
+
 /*
  * This function is called during module intialization to pass module 
parameters
  * for the DWC_otg core.
@@ -1323,6 +1340,7 @@ void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
dwc2_set_param_uframe_sched(hsotg, params->uframe_sched);
dwc2_set_param_external_id_pin_ctl(hsotg, params->external_id_pin_ctl);
dwc2_set_param_hibernation(hsotg, params->hibernation);
+   dwc2_set_param_stm32_powerdown(hsotg, params->stm32_powerdown);
 }
 
 /*
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 3c58d63..d3e4fcb 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -386,6 +386,10 @@ enum dwc2_ep0_state {
  * needed.
  * 0 - No (default)
  * 1 - Yes
+ * @stm32_powerdown:   Enable STM32 specific USB FS transceiver power down
+ * control.
+ * 0 = USB FS transceiver disabled (default)
+ * 1 = USB FS transceiver enabled
  *
  * The following parameters may be specified when starting the module. These
  * parameters define how the DWC_otg controller should be configured. A
@@ -426,6 +430,7 @@ struct dwc2_core_params {
int uframe_sched;
int external_id_pin_ctl;
int hibernation;
+   int stm32_powerdown;
 };
 
 /**
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 2df3d04..4f9bb93 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -118,7 +118,7 @@ static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg 
*hsotg)
 
 static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
 {
-   u32 usbcfg, i2cctl;
+   u32 usbcfg, usbgpio, i2cctl;
int retval = 0;
 
/*
@@ -142,6 +142,16 @@ static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
return retval;
}
}
+
+   if (hsotg->core_params->stm32_powerdown > 0) {
+   dev_dbg(hsotg->dev, "STM32 FS PHY enabling 
transceiver\n");
+   /* STM32 uses the GGPIO register as general core
+* configuration register.
+*/
+   usbgpio = dwc2_readl(hsotg->regs + GGPIO);
+   usbgpio |= STM32_OTG_GCCFG_PWRDWN;
+   dwc2_writel(usbgpio, hsotg->regs + GGPIO);
+   }
}
 
/*
diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
index 281b57b..d5f9294 100644
--- a/drivers/usb/dwc2/hw.h
+++ b/drivers/usb/dwc2/hw.h
@@ -224,6 +224,8 @@
 
 #define GPVNDCTL   HSOTG_REG(0x0034)
 #define GGPIO  HSOTG_REG(0x0038)
+#define STM32_OTG_GCCFG_PWRDWN (1 << 16)
+
 #define GUID   HSOTG_REG(0x003c)
 #define GSNPSIDHSOTG_REG(0x0040)
 #define GHWCFG1HSOTG_REG(0x0044)
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index fc6f525..d806b94 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -84,6 +84,7 @@ static const struct dwc2_core_params params_hi6220 = {
.uframe_sched   = 0,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .stm32_powerdown= 0,
 };
 
 static const struct dwc2_core_params params_bcm2835 = {
@@ -115,6 +116,7 @@ static const struct dwc2_core_params params_bcm2835 = {
.uframe_sched   = 0,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .stm32_powerdown= 0,
 };
 
 static const 

Re: [PATCH v12 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

On 21 June 2016 at 22:50, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
 This patch introduces the usb charger driver based on usb gadget that
 makes an enhancement to a power driver. It works well in practice but
 that requires a system with suitable hardware.

 The basic conception of the usb charger is that, when one usb charger
 is added or removed by reporting from the usb gadget state change or
 the extcon device state change, the usb charger will report to power
 user to set the current limitation.

 The usb charger will register notifiees on the usb gadget or the extcon
 device to get notified the usb charger state. It also supplies the
 notification mechanism to userspace When the usb charger state is changed.

 Power user will register a notifiee on the usb charger to get notified
 by status changes from the usb charger. It will report to power user
 to set the current limitation when detecting the usb charger is added
 or removed from extcon device state or usb gadget state.

 This patch doesn't yet integrate with the gadget code, so some functions
 which rely on the 'gadget' are not completed, that will be implemented
 in the following patches.

 Signed-off-by: Baolin Wang 
 Reviewed-by: Li Jun 
 Tested-by: Li Jun 
 ---
  drivers/usb/gadget/Kconfig   |7 +
  drivers/usb/gadget/udc/Makefile  |1 +
  drivers/usb/gadget/udc/charger.c |  770 
 ++
  include/linux/usb/charger.h  |  191 ++
  include/uapi/linux/usb/charger.h |   31 ++
  5 files changed, 1000 insertions(+)
  create mode 100644 drivers/usb/gadget/udc/charger.c
  create mode 100644 include/linux/usb/charger.h
  create mode 100644 include/uapi/linux/usb/charger.h

 diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
 index 2057add..89f4e9b 100644
 --- a/drivers/usb/gadget/Kconfig
 +++ b/drivers/usb/gadget/Kconfig
 @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
   help
  It supports the serial gadget can be used as a console.

 +config USB_CHARGER
 + bool "USB charger support"
>>>
>>> you didn't build test all possibilities, did you?
>>>
>>> I have a feeling this won't link if USB_GADGET=m. Can you test that?
>>
>> OK.
>>
>>>
 diff --git a/drivers/usb/gadget/udc/charger.c 
 b/drivers/usb/gadget/udc/charger.c
>>> [...]
 +struct class *usb_charger_class;
>>>
>>> We already have a UDC class, do we really, really need another class
>>> here?
>>
>> We want to manage the usb charger devices by this class and one usb
>> charger device is not equal with one usb device which managed by UDC
>
> Can you explain this statement? If charging is done via a USB peripheral
> port, why don't we have a 1:1 mapping between charger and UDC?

Sorry for confusing, but forget it, I plan to remove the device/class
things as you suggested.

>
>> class, so can we use UDC class to manage charger devices?
>> By the way, you also suggested to use the 'class' things instead of
>> 'bus' in previous mail.
>
> true, I did say that. It seems clearer, however, that we don't need this
> virtual device here.

OK.

>
 +subsys_initcall(usb_charger_class_init);
>>>
>>> this should always work as module_init(). Please make sure that's the
>>> case.
>>
>> we should make sure the charger class has been allocated before user
>> try to add a new gadget to the udc class. So it should be issued
>> before 'module_init()' (many usb drivers are at module_init level).
>
> -EPROBE_DEFER?

OK. But 'EPROBE_DEFER' is not needed if we remove the class things.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 20:53, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

 We can get/put usb charger and mange usb charger attributes with the
 device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.

Make sense.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/12] doc: binding: pwrseq-usb-generic: add binding doc for generic usb power sequence driver

[Peter Chen]

On Tue, Jun 21, 2016 at 04:26:53PM -0500, Rob Herring wrote:
> On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
> > On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> > > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen  
> > > > > wrote:
> > > > > > Add binding doc for generic usb power sequence driver, and update
> > > > > > generic usb device binding-doc accordingly.
> 
> [...]
> 
> > > > clocks = < IMX6SX_CLK_CKO>;
> > > > 
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > > ethernet: asix@1 {
> > > > compatible = "usbb95,1708";
> > > > reg = <1>;
> > > > 
> > > > power-sequence;
> > > > reset-gpios = < 6 GPIO_ACTIVE_LOW>; /* 
> > > > ethernet_rst */
> > > > reset-duration-us = <15>;
> > > > clocks = < IMX6SX_CLK_IPG>;
> > > > };
> > > > };
> > > > };
> > > > 
> > > > If the node has property "power-sequence", the pwrseq core will create
> > > > related platform device, and the driver under pwrseq driver will handle
> > > > power sequence stuffs. 
> > > 
> > > This I have issue with. If you are creating a platform device here, you 
> > > are trying to work-around limitations in the linux driver model.
> > 
> > My current solution like below, but it seems you didn't agree with that.
> > I just double confirm here, if you don't, I give up the solution for
> > using generic power sequence framework.
> > 
> > In drivers/usb/core/hub.c
> > 
> > for_each_child_of_node(parent->of_node, node) {
> > hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
> > if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
> > pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
> > if (!pwrseq_node) {
> > ret = -ENOMEM;
> > goto err1;
> > }
> > /* power on sequence */
> > ret = pwrseq_pre_power_on(hdev_pwrseq);
> 
> Why does this function need to do anything more than:
> 
> - Check if the child has a "power-sequence" property
> - Get the "reset-gpios" GPIO
> - Assert reset for specified/default time
> - Deassert reset
> 

Besides gpios, it may exist clock and regulator operation, and the
operation may be failed. I thought these operations can be easy
done belongs to a device, but now, let me try this straight-forward
way, thanks.

Peter
> Then continue on as normal. That seems straight-forward to me.
> 
> There is no reason you need a platform device in the mix. Perhaps trying 
> to move the MMC pwr-seq code is pointless as it adds needless 
> complexity.
> 
> [...]
> 
> > > Either 
> > > we need some sort of pre-probe hook to the drivers to call or each 
> > > parent node driver is responsible for checking and calling pwr-seq 
> > > functions for child nodes. e.g. The host controller calls pwr-seq for 
> > > the hub, the hub driver calls the power seq for the asix chip. Soon as 
> > > we have a case too complex for the generic pwr-seq, we're going to need 
> > > the pre-probe hook as I don't want to see a continual expansion of 
> > > generic pwr-seq binding for ever more complex cases.
> > > 
> > 
> > How the driver know what it needs to handle (eg, gpio, clock) if there
> > is no device for it? The most important we need to consider is which
> > device owns there power sequence properties, then the corresponding
> > driver can handle it.
> 
> What can be handled by is defined by presence of power-sequence 
> property. There can be 1 driver for the device. That is the USB hub 
> driver in this example. You should not have 2 "devices".
> 
> Rob

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] usb: gadget: composite: ability for USB functions to process control requests in config 0

[Felix Hädicke]

It can sometimes be necessary for gadget drivers to process non-standard
control requests, which host devices can send without having sent
USB_REQ_SET_CONFIGURATION.

Therefore, the req_match() usb_function method is enhanced with the new
parameter "config0". When a USB configuration is active, this parameter
is false. When a non-core control request is processed in
composite_setup(), without an active configuration, req_match() of the
USB functions of all available configurations which implement this
function, is called with config0=true. Then the control request gets
processed by the first usb_function instance whose req_match() returns
true.

Signed-off-by: Felix Hädicke 
---
 drivers/usb/gadget/composite.c  | 16 ++--
 drivers/usb/gadget/function/f_fs.c  |  9 +++--
 drivers/usb/gadget/function/f_printer.c |  6 +-
 include/linux/usb/composite.h   |  3 ++-
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 524e233..81ed5a8 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1872,17 +1872,21 @@ unknown:
/* functions always handle their interfaces and endpoints...
 * punt other recipients (other, WUSB, ...) to the current
 * configuration code.
-*
-* REVISIT it could make sense to let the composite device
-* take such requests too, if that's ever needed:  to work
-* in config 0, etc.
 */
if (cdev->config) {
list_for_each_entry(f, >config->functions, list)
-   if (f->req_match && f->req_match(f, ctrl))
+   if (f->req_match &&
+   f->req_match(f, ctrl, false))
goto try_fun_setup;
-   f = NULL;
+   } else {
+   struct usb_configuration *c;
+   list_for_each_entry(c, >configs, list)
+   list_for_each_entry(f, >functions, list)
+   if (f->req_match &&
+   f->req_match(f, ctrl, true))
+   goto try_fun_setup;
}
+   f = NULL;
 
switch (ctrl->bRequestType & USB_RECIP_MASK) {
case USB_RECIP_INTERFACE:
diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 9eb4003..4cf6efc 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -99,7 +99,8 @@ static void ffs_func_disable(struct usb_function *);
 static int ffs_func_setup(struct usb_function *,
  const struct usb_ctrlrequest *);
 static bool ffs_func_req_match(struct usb_function *,
-  const struct usb_ctrlrequest *);
+  const struct usb_ctrlrequest *,
+  bool config0);
 static void ffs_func_suspend(struct usb_function *);
 static void ffs_func_resume(struct usb_function *);
 
@@ -3016,10 +3017,14 @@ static int ffs_func_setup(struct usb_function *f,
 }
 
 static bool ffs_func_req_match(struct usb_function *f,
-  const struct usb_ctrlrequest *creq)
+  const struct usb_ctrlrequest *creq,
+  bool config0)
 {
struct ffs_function *func = ffs_func_from_usb(f);
 
+   if (config0)
+   return false;
+
switch (creq->bRequestType & USB_RECIP_MASK) {
case USB_RECIP_INTERFACE:
return ffs_func_revmap_intf(func,
diff --git a/drivers/usb/gadget/function/f_printer.c 
b/drivers/usb/gadget/function/f_printer.c
index c45104e..aebffc6 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -897,13 +897,17 @@ static void printer_soft_reset(struct printer_dev *dev)
 /*-*/
 
 static bool gprinter_req_match(struct usb_function *f,
-  const struct usb_ctrlrequest *ctrl)
+  const struct usb_ctrlrequest *ctrl,
+  bool config0)
 {
struct printer_dev  *dev = func_to_printer(f);
u16 w_index = le16_to_cpu(ctrl->wIndex);
u16 w_value = le16_to_cpu(ctrl->wValue);
u16 w_length = le16_to_cpu(ctrl->wLength);
 
+   if (config0)
+   return false;
+
if ((ctrl->bRequestType & USB_RECIP_MASK) != USB_RECIP_INTERFACE ||
(ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS)
return false;
diff --git 

[PATCH 3/3] usb: gadget: f_fs: handle control requests in config 0

[Felix Hädicke]

Introduces a new FunctionFS descriptor flag named
FUNCTIONFS_CONFIG0_SETUP.

When this flag is enabled, FunctionFS userspace drivers can process
non-standard control requests in configuration 0.

Signed-off-by: Felix Hädicke 
---
 drivers/usb/gadget/function/f_fs.c  | 5 +++--
 include/uapi/linux/usb/functionfs.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 4cf6efc..43aeed6 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2126,7 +2126,8 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
  FUNCTIONFS_HAS_MS_OS_DESC |
  FUNCTIONFS_VIRTUAL_ADDR |
  FUNCTIONFS_EVENTFD |
- FUNCTIONFS_ALL_CTRL_RECIP)) {
+ FUNCTIONFS_ALL_CTRL_RECIP |
+ FUNCTIONFS_CONFIG0_SETUP)) {
ret = -ENOSYS;
goto error;
}
@@ -3022,7 +3023,7 @@ static bool ffs_func_req_match(struct usb_function *f,
 {
struct ffs_function *func = ffs_func_from_usb(f);
 
-   if (config0)
+   if (config0 && !(func->ffs->user_flags & FUNCTIONFS_CONFIG0_SETUP))
return false;
 
switch (creq->bRequestType & USB_RECIP_MASK) {
diff --git a/include/uapi/linux/usb/functionfs.h 
b/include/uapi/linux/usb/functionfs.h
index 93da4ca..acc6369 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -22,6 +22,7 @@ enum functionfs_flags {
FUNCTIONFS_VIRTUAL_ADDR = 16,
FUNCTIONFS_EVENTFD = 32,
FUNCTIONFS_ALL_CTRL_RECIP = 64,
+   FUNCTIONFS_CONFIG0_SETUP = 128,
 };
 
 /* Descriptor of an non-audio endpoint */
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usb: gadget: f_fs: handle control requests not directed to interface or endpoint

[Felix Hädicke]

Introduces a new FunctionFS descriptor flag named
FUNCTIONFS_ALL_CTRL_RECIP. When this flag is enabled, control requests,
which are not explicitly directed to an interface or endpoint, can be
handled.

This allows FunctionFS userspace drivers to process non-standard
control requests.

Signed-off-by: Felix Hädicke 
---
 drivers/usb/gadget/function/f_fs.c  | 34 ++
 include/uapi/linux/usb/functionfs.h |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 73515d5..9eb4003 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -98,6 +98,8 @@ static int ffs_func_set_alt(struct usb_function *, unsigned, 
unsigned);
 static void ffs_func_disable(struct usb_function *);
 static int ffs_func_setup(struct usb_function *,
  const struct usb_ctrlrequest *);
+static bool ffs_func_req_match(struct usb_function *,
+  const struct usb_ctrlrequest *);
 static void ffs_func_suspend(struct usb_function *);
 static void ffs_func_resume(struct usb_function *);
 
@@ -2122,7 +2124,8 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
  FUNCTIONFS_HAS_SS_DESC |
  FUNCTIONFS_HAS_MS_OS_DESC |
  FUNCTIONFS_VIRTUAL_ADDR |
- FUNCTIONFS_EVENTFD)) {
+ FUNCTIONFS_EVENTFD |
+ FUNCTIONFS_ALL_CTRL_RECIP)) {
ret = -ENOSYS;
goto error;
}
@@ -2974,8 +2977,9 @@ static int ffs_func_setup(struct usb_function *f,
 * handle them.  All other either handled by composite or
 * passed to usb_configuration->setup() (if one is set).  No
 * matter, we will handle requests directed to endpoint here
-* as well (as it's straightforward) but what to do with any
-* other request?
+* as well (as it's straightforward).  Other request recipient
+* types are only handled when the user flag FUNCTIONFS_ALL_CTRL_RECIP
+* is being used.
 */
if (ffs->state != FFS_ACTIVE)
return -ENODEV;
@@ -2996,7 +3000,10 @@ static int ffs_func_setup(struct usb_function *f,
break;
 
default:
-   return -EOPNOTSUPP;
+   if (func->ffs->user_flags & FUNCTIONFS_ALL_CTRL_RECIP)
+   ret = le16_to_cpu(creq->wIndex);
+   else
+   return -EOPNOTSUPP;
}
 
spin_lock_irqsave(>ev.waitq.lock, flags);
@@ -3008,6 +3015,24 @@ static int ffs_func_setup(struct usb_function *f,
return 0;
 }
 
+static bool ffs_func_req_match(struct usb_function *f,
+  const struct usb_ctrlrequest *creq)
+{
+   struct ffs_function *func = ffs_func_from_usb(f);
+
+   switch (creq->bRequestType & USB_RECIP_MASK) {
+   case USB_RECIP_INTERFACE:
+   return ffs_func_revmap_intf(func,
+   le16_to_cpu(creq->wIndex) >= 0);
+   case USB_RECIP_ENDPOINT:
+   return ffs_func_revmap_ep(func,
+ le16_to_cpu(creq->wIndex) >= 0);
+   default:
+   return (bool) (func->ffs->user_flags &
+  FUNCTIONFS_ALL_CTRL_RECIP);
+   }
+}
+
 static void ffs_func_suspend(struct usb_function *f)
 {
ENTER();
@@ -3258,6 +3283,7 @@ static struct usb_function *ffs_alloc(struct 
usb_function_instance *fi)
func->function.set_alt = ffs_func_set_alt;
func->function.disable = ffs_func_disable;
func->function.setup   = ffs_func_setup;
+   func->function.req_match = ffs_func_req_match;
func->function.suspend = ffs_func_suspend;
func->function.resume  = ffs_func_resume;
func->function.free_func = ffs_free;
diff --git a/include/uapi/linux/usb/functionfs.h 
b/include/uapi/linux/usb/functionfs.h
index 108dd79..93da4ca 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -21,6 +21,7 @@ enum functionfs_flags {
FUNCTIONFS_HAS_MS_OS_DESC = 8,
FUNCTIONFS_VIRTUAL_ADDR = 16,
FUNCTIONFS_EVENTFD = 32,
+   FUNCTIONFS_ALL_CTRL_RECIP = 64,
 };
 
 /* Descriptor of an non-audio endpoint */
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] USB Type-C Connector class

[Guenter Roeck]

On Tue, Jun 21, 2016 at 05:51:49PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> I'm considering all the RFCs I send after v1 as v2 (I don't remember
> how many I send). Hope this is OK and hope there is nothing big
> missing anymore (or broken).
> 
> Sorry about the delay. I've been really busy with some internal tasks.
> I'm guessing we missed v4.8 with this thing. I'm sorry about that.
> 
> I'm including in this series a driver for the Broxton PMIC USB Type-C
> PHY.
> 
Can you by any chance push the current version into your repository
on github ?

Thanks,
Guenter

> 
> Changes since v2:
> - Notification on role and alternate mode changes
> - cleanups
> 
> Changes since v1:
> - Completely rewrote alternate mode support
> - Patners, cables and cable plugs presented as devices.
> 
> 
> Heikki Krogerus (2):
>   usb: USB Type-C connector class
>   usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY
> 
>  Documentation/ABI/testing/sysfs-class-typec |  163 
>  Documentation/usb/typec.txt |  101 +++
>  MAINTAINERS |9 +
>  drivers/usb/Kconfig |2 +
>  drivers/usb/Makefile|2 +
>  drivers/usb/typec/Kconfig   |   21 +
>  drivers/usb/typec/Makefile  |2 +
>  drivers/usb/typec/typec.c   | 1173 
> +++
>  drivers/usb/typec/typec_wcove.c |  376 +
>  include/linux/usb/typec.h   |  255 ++
>  10 files changed, 2104 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 drivers/usb/typec/typec_wcove.c
>  create mode 100644 include/linux/usb/typec.h
> 
> -- 
> 2.8.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Guenter Roeck]

On Tue, Jun 21, 2016 at 09:43:20PM +0200, Oliver Neukum wrote:
> On Tue, 2016-06-21 at 06:24 -0700, Guenter Roeck wrote:
> > On 06/21/2016 06:08 AM, Oliver Neukum wrote:
> > > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > >> The purpose of this class is to provide unified interface for user
> > >> space to get the status and basic information about USB Type-C
> > >> Connectors in the system, control data role swapping, and when USB PD
> > >> is available, also power role swapping and Alternate Modes.
> > >
> > > This raises two more questions.
> > >
> > > 1. Booting
> > >
> > > It is possible that our only display and, worse, our source
> > > of power is a display that can be used only in an alternate mode
> > > and is connected via a type C connector.
> > >
> > > We need some kind of boot time support for alternate modes.
> > >
> > > The firmware will surely want to display something. So it is possible
> > > that we start the OS will a valid power contract. How do we deal
> > > with that? Renegotiate?
> > >
> > 
> > In one of my drivers, the PD protocol is running on an EC and the Linux
> 
> Is that code public?
> 
Pretty much, though it is not always up to date. It isn't pushed into an
official chromiumos kernel branch yet, but it is available in the repository
if one knows what to look for.

Unfortunately, it looks like I lost permission to create a sandbox in the
repository. I'll see what I can do to get that fixed.

> > driver is just interfacing it to the typec class. I don't do any 
> > renegotiating
> > but just report the port state to the class. What is wrong with that, and 
> > why
> > would it not work ?
> 
> It is not wrong. But it is unclear how it would work if we pass control
> between boot loader and OS. If I read the spec for TCPM and TCPC
> correctly, we can read back the voltage, but not the agreed currents,
> maximum currents and suspend rules. Yet we need those for power
> budgeting.
> 
The TCPC would not (necessarily) know that information, unfortunately.
I am not even sure about the negotiated voltage - are you sure the TCPC
would know it ?

> We could reset, but then we leave the alternate mode.
> 
Correct. I wasn't able to find a solution for that problem (yet). Going forward,
I can see that being an issue, for example if the BIOS/ROMMON negotiates a
contract but the TCPM running in the kernel doesn't know about it and thus has
to disconnect and reconnect the port.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/12] doc: binding: pwrseq-usb-generic: add binding doc for generic usb power sequence driver

[Rob Herring]

On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
> On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen  wrote:
> > > > > Add binding doc for generic usb power sequence driver, and update
> > > > > generic usb device binding-doc accordingly.

[...]

> > >   clocks = < IMX6SX_CLK_CKO>;
> > > 
> > >   #address-cells = <1>;
> > >   #size-cells = <0>;
> > >   ethernet: asix@1 {
> > >   compatible = "usbb95,1708";
> > >   reg = <1>;
> > > 
> > >   power-sequence;
> > >   reset-gpios = < 6 GPIO_ACTIVE_LOW>; /* 
> > > ethernet_rst */
> > >   reset-duration-us = <15>;
> > >   clocks = < IMX6SX_CLK_IPG>;
> > >   };
> > >   };
> > > };
> > > 
> > > If the node has property "power-sequence", the pwrseq core will create
> > > related platform device, and the driver under pwrseq driver will handle
> > > power sequence stuffs. 
> > 
> > This I have issue with. If you are creating a platform device here, you 
> > are trying to work-around limitations in the linux driver model.
> 
> My current solution like below, but it seems you didn't agree with that.
> I just double confirm here, if you don't, I give up the solution for
> using generic power sequence framework.
> 
> In drivers/usb/core/hub.c
> 
>   for_each_child_of_node(parent->of_node, node) {
>   hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
>   if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
>   pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
>   if (!pwrseq_node) {
>   ret = -ENOMEM;
>   goto err1;
>   }
>   /* power on sequence */
>   ret = pwrseq_pre_power_on(hdev_pwrseq);

Why does this function need to do anything more than:

- Check if the child has a "power-sequence" property
- Get the "reset-gpios" GPIO
- Assert reset for specified/default time
- Deassert reset

Then continue on as normal. That seems straight-forward to me.

There is no reason you need a platform device in the mix. Perhaps trying 
to move the MMC pwr-seq code is pointless as it adds needless 
complexity.

[...]

> > Either 
> > we need some sort of pre-probe hook to the drivers to call or each 
> > parent node driver is responsible for checking and calling pwr-seq 
> > functions for child nodes. e.g. The host controller calls pwr-seq for 
> > the hub, the hub driver calls the power seq for the asix chip. Soon as 
> > we have a case too complex for the generic pwr-seq, we're going to need 
> > the pre-probe hook as I don't want to see a continual expansion of 
> > generic pwr-seq binding for ever more complex cases.
> > 
> 
> How the driver know what it needs to handle (eg, gpio, clock) if there
> is no device for it? The most important we need to consider is which
> device owns there power sequence properties, then the corresponding
> driver can handle it.

What can be handled by is defined by presence of power-sequence 
property. There can be 1 driver for the device. That is the USB hub 
driver in this example. You should not have 2 "devices".

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Tue, 2016-06-21 at 16:58 +0300, Heikki Krogerus wrote:
> On Tue, Jun 21, 2016 at 03:08:52PM +0200, Oliver Neukum wrote:
>  
> > The firmware will surely want to display something. So it is possible
> > that we start the OS will a valid power contract. How do we deal
> > with that? Renegotiate?
> 
> Systems where the firmware has to negotiate PD will likely provide
> firmware interface like UCSI, and where the OS has no direct
> interaction with the USB PD transceiver. In these case there is no
> need to renegotiate as we are just reporting in OS the initial state
> after bootup.

How certain is that? I was under the impression that on many systems
the OS would speak to the TCPM directly.

> We do have a system where the typec port is used to power the board.
> On these systems the firmware does not communicate PD (so we will
> never have the firmware displaying anything over Type-C on those
> systems), but the USB PD chargers for example are detected as 3.0A
> Type-C power supplies before any USB PD negotiation takes place, just
> like the spec says, and that is more then enough to power these boards.

Now correct me, if I am misreading the spec. I am sure the system
will boot unless it needs ridiculous amounts of power, but
will we see anything on the screen? As far as I can tell the spec
actually says that you cannot enter an alternate mode without having
established a power contract.
If we really leave entering modes up to user space, we have lost
printk before getting into the initrd at the earliest.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Tue, 2016-06-21 at 17:51 +0300, Heikki Krogerus wrote:
> +What:  /sys/class/typec//supported_data_roles
> +Data:  June 2016
> +Contact:   Heikki Krogerus 
> +Description:
> +   Lists the USB data roles, host or device, the port is
> capable
> +   of supporting.

On third thought, this is a problem. Looking at 4.4.8.1
DEVICE_CAPABILITIES (Required) of USB Type-C Port Controller
Interface Specification we lack capability.

A port that can do DRP is not the same thing as a port that
can be switched between DFP and UFP. We cannot express that.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Tue, 2016-06-21 at 06:24 -0700, Guenter Roeck wrote:
> On 06/21/2016 06:08 AM, Oliver Neukum wrote:
> > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> >> The purpose of this class is to provide unified interface for user
> >> space to get the status and basic information about USB Type-C
> >> Connectors in the system, control data role swapping, and when USB PD
> >> is available, also power role swapping and Alternate Modes.
> >
> > This raises two more questions.
> >
> > 1. Booting
> >
> > It is possible that our only display and, worse, our source
> > of power is a display that can be used only in an alternate mode
> > and is connected via a type C connector.
> >
> > We need some kind of boot time support for alternate modes.
> >
> > The firmware will surely want to display something. So it is possible
> > that we start the OS will a valid power contract. How do we deal
> > with that? Renegotiate?
> >
> 
> In one of my drivers, the PD protocol is running on an EC and the Linux

Is that code public?

> driver is just interfacing it to the typec class. I don't do any renegotiating
> but just report the port state to the class. What is wrong with that, and why
> would it not work ?

It is not wrong. But it is unclear how it would work if we pass control
between boot loader and OS. If I read the spec for TCPM and TCPC
correctly, we can read back the voltage, but not the agreed currents,
maximum currents and suspend rules. Yet we need those for power
budgeting.

We could reset, but then we leave the alternate mode.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH] usb: dwc2: Add reset control to dwc2

[Dinh Nguyen]

On 06/21/2016 02:15 PM, John Youn wrote:
> 
> Can you resend this to Philipp Zabel.
> 
> Hi Philipp,
> 
> Could you take this in your reset/next tree? It depends on the
> following patch there:
> 
> http://marc.info/?l=linux-usb=146473891018262=2
> 

I've resent that patch to Philipp.

Thanks,
Dinh

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH] usb: dwc2: Add reset control to dwc2

[John Youn]

On 6/21/2016 1:06 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> On 6/3/2016 8:59 AM, dingu...@opensource.altera.com wrote:
>>> From: Dinh Nguyen 
>>>
>>> Allow for platforms that have a reset controller driver in place to bring
>>> the USB IP out of reset.
>>>
>>> Signed-off-by: Dinh Nguyen 
>>> Acked-by: John Youn 
>>> Tested-by: Stefan Wahren 
>>> ---
>>> v7: Use devm_reset_control_get_optional()
>>> v6: fix 80 line checkpatch warning in dev_err print
>>> v5: updated error conditions for not finding the reset property
>>> v4: use dev_dbg() if not a -EPROBE_DEFER
>>> v3: fix compile error
>>> v2: move to lowlevel_hw_init()
>>> ---
>>>  drivers/usb/dwc2/core.h |1 +
>>>  drivers/usb/dwc2/platform.c |   22 ++
>>>  2 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 3c58d63..f748132 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -837,6 +837,7 @@ struct dwc2_hsotg {
>>> void *priv;
>>> int irq;
>>> struct clk *clk;
>>> +   struct reset_control *reset;
>>>  
>>> unsigned int queuing_high_bandwidth:1;
>>> unsigned int srp_success:1;
>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>> index 88629be..d34f169 100644
>>> --- a/drivers/usb/dwc2/platform.c
>>> +++ b/drivers/usb/dwc2/platform.c
>>> @@ -45,6 +45,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #include 
>>>  
>>> @@ -337,6 +338,24 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg 
>>> *hsotg)
>>>  {
>>> int i, ret;
>>>  
>>> +   hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
>>> +   if (IS_ERR(hsotg->reset)) {
>>> +   ret = PTR_ERR(hsotg->reset);
>>> +   switch (ret) {
>>> +   case -ENOENT:
>>> +   case -ENOTSUPP:
>>> +   hsotg->reset = NULL;
>>> +   break;
>>> +   default:
>>> +   dev_err(hsotg->dev, "error getting reset control %d\n",
>>> +   ret);
>>> +   return ret;
>>> +   }
>>> +   }
>>> +
>>> +   if (hsotg->reset)
>>> +   reset_control_deassert(hsotg->reset);
>>> +
>>> /* Set default UTMI width */
>>> hsotg->phyif = GUSBCFG_PHYIF16;
>>>  
>>> @@ -434,6 +453,9 @@ static int dwc2_driver_remove(struct platform_device 
>>> *dev)
>>> if (hsotg->ll_hw_enabled)
>>> dwc2_lowlevel_hw_disable(hsotg);
>>>  
>>> +   if (hsotg->reset)
>>> +   reset_control_assert(hsotg->reset);
>>> +
>>> return 0;
>>>  }
>>>  
>>>
>>
>> Hi Felipe,
>>
>> This patch depends on this one in the reset control tree.
>>
>> http://marc.info/?l=linux-usb=146473891018262=2
>>
>> Should you also have this patch in your tree? Or what is the best way
>> to deal with it?
> 
> We have two ways of dealing with this:
> 
> 1. We wait for v4.9
> 2. Reset folks take this patch together with the rest of the changes
> 
> I'm fine with either way. Should (2) be chosen here's my ack:
> 
> Acked-by: Felipe Balbi 
> 

Hi Dinh,

Can you resend this to Philipp Zabel.

Hi Philipp,

Could you take this in your reset/next tree? It depends on the
following patch there:

http://marc.info/?l=linux-usb=146473891018262=2

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: renesas_usbhs: make usbhs_write32() static

[Ben Dooks]

The usbhs_write32 function is not used outside of the rcar3.c
file, so fix the following sparse warning by making it static:

drivers/usb/renesas_usbhs/rcar3.c:26:6: warning: symbol 'usbhs_write32' was not 
declared. Should it be static?

Signed-off-by: Ben Dooks 
---
Cc: Yoshihiro Shimoda 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Cc: linux-renesas-...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
---
 drivers/usb/renesas_usbhs/rcar3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/renesas_usbhs/rcar3.c 
b/drivers/usb/renesas_usbhs/rcar3.c
index 38b01f2..1d70add 100644
--- a/drivers/usb/renesas_usbhs/rcar3.c
+++ b/drivers/usb/renesas_usbhs/rcar3.c
@@ -23,7 +23,7 @@
 #define UGCTRL2_RESERVED_3 0x0001  /* bit[3:0] should be B'0001 */
 #define UGCTRL2_USB0SEL_OTG0x0030
 
-void usbhs_write32(struct usbhs_priv *priv, u32 reg, u32 data)
+static void usbhs_write32(struct usbhs_priv *priv, u32 reg, u32 data)
 {
iowrite32(data, priv->base + reg);
 }
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 01:02:59PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> >> >> So far, I haven't seen anybody talking about real USB OTG (the spec)
> >> >> when they say OTG. Usually they just mean "a method for swapping between
> >> >> host and peripheral roles, but we really don't want all the extra cost
> >> >> of the OTG specification".
> >> >> 
> >> >
> >> > That's what I thought before, but the request from the Marketing guy is
> >> > "To prove the SoC is OTG compliance, support HNP and SRP", don't you
> >> > see the SoC reference manual say "it supports HNP and SRP"?
> >> >
> >> > If there is no request, who else wants to implement so complicated FSM
> >> > but seldom use cases, and go to pass OTG compliance test (tested by PET).
> >> 
> >> I stand corrected :-)
> >> 
> >> So there is one user for this layer. And this user has its own role
> >> control registers. I'm not convinced we need this large generic layer
> >> for one user.
> >> 
> >
> > You mean chipidea or dwc3? I have more comments below.
> 
> chipidea. From the point of OTG (or DRD) dwc3 is very
> self-sufficient. HW itself tracks state machine, much like MUSB does.

You mean HW can do state machine switch? If we are A device,
- Does the hardware knows if B device is HNP enabled or not?
- And if B device is HNP enabled, does it can switch itself from host
to peripheral when the B device is disconnected (a_suspend->a_peripheral)

Does hardware can really follow Figure 7-1: OTG A-device with HNP State
Diagram at On-The-Go and Embedded Host Supplement to the USB Revision
2.0 Specification? And can pass PET test?

> 
> >> >> > For the real use case, some Carplay platforms need it.
> >> >> 
> >> >> Carplay does *NOT* rely on OTG. Apple has its own proprietary and closed
> >> >> specification which is not OTG-compliant.
> >> >> 
> >> >
> >> > Yes, it is not OTG-compliant, but it can co-work with some standard OTG 
> >> > FSM
> >> > states to finish role swap.
> >> 
> >> What are you referring to as "finish role swap"? I don't get that.
> >
> > Change current role from host to peripheral.
> 
> Okay, we have two scenarios here:
> 
> 1. You need full OTG compliance
> 
>   For this, granted, you need the state machine if your HW doesn't
>   track it. This is a given. With only one user, however, perhaps
>   we don't need a generic layer. There are not enough different
>   setups to design a good enough generic layer. We will end up
>   with a pseudo-generic framework which is coupled with its only
>   user.
> 
> 2. Dual-role support, without OTG compliance
> 
>   In this case, you don't need a stack. All you need is a signal
>   to tell you state of ID pin and another to tell you state of
>   VBUS level. If you have those, you don't need to walk an OTG
>   state machine at all. You don't need any of those quirky OTG
>   timers, agreed?
> 
>   Given the above, why would you even want to use a subset of OTG
>   state machine to implement something that's _usually_ as simple
>   as:
> 
> 8<--
>   vbus = read(VBUS_STATE); /* could be a gpio_get_value() */
> id = read(ID_STATE); /* could be a gpio_get_value() */
> 
> set_role(id);
> set_vbus(vbus);
> 
> 

In fact, the individual driver can do it by itself. The chipidea driver
handles OTG and dual-role well currently. By considering this OTG/DRD
framework is worthwhile or not, we would like to see if it can
simplify DRD design for each driver, and can benefit the platforms which
has different drivers for host and peripheral to finish the role switch
well.

- The common start/stop host and peripheral operation
eg, when switch from host to peripheral, all drivers can use
usb_remove_hcd to finish it.
- A common workqueue to handle vbus and id event
- sysfs for role switch

> >> > Notice, it needs to swap role without disconnect cable.
> >> 
> >> right, I can swap role without changing cable, but that's not OTG. The
> >> mechanism for that, AFAICT, is not HNP. I don't know details about
> >> CarPlay because the spec isn't public, but my understanding is that
> >> CarPlay doesn't rely on anything from OTG spec.
> >
> > Since it is non-public, I can't say much. Some flows of its role-swap
> > refers to On-The-Go and Embedded Host Supplement to the USB Revision 2.0
> > Specification. 
> >
> > But OTG FSM is not the only way, the platform which can do role-swap
> > without disconnection can support it too.
> 
> Right, all you need for CarPlay is what I wrote above. You don't need
> full OTG compliance for that, right?
> 

OTG FSM is not necessary, other dual-role switch also can satisfy its
requirement.

> >> 
> >> > Consider the use case the host driver is at host/ and udc driver is
> >> > at gadget/udc, how to finish to role swap?
> >> 
> >> 

Re: [PATCH v4] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Alan Stern]

On Tue, 21 Jun 2016, Nicolas Ferre wrote:

> Le 21/06/2016 03:32, Wenyou Yang a �crit :
> > In order to save power consumption, as a workaround, forcibly suspend
> > the USB PORTA/B/C via setting the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register in the SFR while OHCI USB suspend.
> > 
> > This suspend operation must be done before the USB clock is disabled,
> > resume after the USB clock is enabled.
> > 
> > Signed-off-by: Wenyou Yang 
> 
> Acked-by: Nicolas Ferre 
> 
> Alan, can you take it as it doesn't have dependency on the at91 material
> anymore.

Sorry, I will not accept this patch until Wenyou Yang answers the 
questions I raised in

http://marc.info/?l=linux-usb=146307669102601=2

and

http://marc.info/?l=linux-usb=146541324027407=2

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Nicolas Ferre]

Le 21/06/2016 03:32, Wenyou Yang a écrit :
> In order to save power consumption, as a workaround, forcibly suspend
> the USB PORTA/B/C via setting the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register in the SFR while OHCI USB suspend.
> 
> This suspend operation must be done before the USB clock is disabled,
> resume after the USB clock is enabled.
> 
> Signed-off-by: Wenyou Yang 

Acked-by: Nicolas Ferre 

Alan, can you take it as it doesn't have dependency on the at91 material
anymore.

Thanks, bye.

> ---
> 
> Changes in v4:
>  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
>is present or not to decide if this feature is applied or not
>when USB OHCI suspend/resume, instead of new compatible.
>  - Drop the compatible "atmel,sama5d2-ohci".
>  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
>ohci node.
>  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
>atmel-sfr.h which already exists.
>  - Change the defines to align the exists.
> 
> Changes in v3:
>  - Change the compatible description for more precise.
> 
> Changes in v2:
>  - Add compatible to support forcibly suspend the ports.
>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>  - Add error checking for .sfr_regmap.
>  - Remove unnecessary regmap_read() statement.
> 
>  drivers/usb/host/ohci-at91.c | 55 
> 
>  include/soc/at91/atmel-sfr.h | 13 +++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index d177372..f07c398 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -21,8 +21,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
> +#include 
>  
>  #include "ohci.h"
>  
> @@ -51,6 +54,7 @@ struct ohci_at91_priv {
>   struct clk *hclk;
>   bool clocked;
>   bool wakeup;/* Saved wake-up state for resume */
> + struct regmap *sfr_regmap;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -132,6 +136,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + regmap = NULL;
> +
> + return regmap;
> +}
> +
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
>  /* configure so an HC device and id are always provided */
> @@ -197,6 +212,10 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>  
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> + if (!ohci_at91->sfr_regmap)
> + dev_warn(dev, "failed to find sfr node\n");
> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;
> @@ -581,6 +600,38 @@ static int ohci_hcd_at91_drv_remove(struct 
> platform_device *pdev)
>   return 0;
>  }
>  
> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (!regmap)
> + return 0;
> +
> + ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> + if (ret)
> + return ret;
> +
> + if (enable)
> + regval &= ~AT91_OHCIICR_USB_SUSPEND;
> + else
> + regval |= AT91_OHCIICR_USB_SUSPEND;
> +
> + regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> +
> + return 0;
> +}
> +
> +static int ohci_at91_port_suspend(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, false);
> +}
> +
> +static int ohci_at91_port_resume(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, true);
> +}
> +
>  static int __maybe_unused
>  ohci_hcd_at91_drv_suspend(struct device *dev)
>  {
> @@ -618,6 +669,8 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   ohci_writel(ohci, ohci->hc_control, >regs->control);
>   ohci->rh_state = OHCI_RH_HALTED;
>  
> + ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> +
>   /* flush the writes */
>   (void) ohci_readl (ohci, >regs->control);
>   at91_stop_clock(ohci_at91);
> @@ -637,6 +690,8 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>  
>   at91_start_clock(ohci_at91);
>  
> + ohci_at91_port_resume(ohci_at91->sfr_regmap);
> +
>   ohci_resume(hcd, false);
>   return 0;
>  }
> diff --git a/include/soc/at91/atmel-sfr.h b/include/soc/at91/atmel-sfr.h
> index 2f9bb98..cca28d4 100644
> --- a/include/soc/at91/atmel-sfr.h
> +++ b/include/soc/at91/atmel-sfr.h
> @@ -13,6 +13,19 @@
>  #ifndef _LINUX_MFD_SYSCON_ATMEL_SFR_H
>  #define 

Re: [PATCH v4] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Alexandre Belloni]

On 21/06/2016 at 09:32:44 +0800, Wenyou Yang wrote :
> In order to save power consumption, as a workaround, forcibly suspend
> the USB PORTA/B/C via setting the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register in the SFR while OHCI USB suspend.
> 
> This suspend operation must be done before the USB clock is disabled,
> resume after the USB clock is enabled.
> 
> Signed-off-by: Wenyou Yang 
Reviewed-by: Alexandre Belloni 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, Jun 21, 2016 at 02:50:27PM +0300, Felipe Balbi wrote:
> Mark Brown  writes:

> > The wm831x has no DT support currently.

> okay, perhaps its time to add it.

The only platform using it would need the DT connector overlays
completing in order to be able to convert to DT.  I'm really not
convinced it's worth the effort either.


signature.asc
Description: PGP signature

[PATCHv3 0/2] USB Type-C Connector class

[Heikki Krogerus]

Hi,

I'm considering all the RFCs I send after v1 as v2 (I don't remember
how many I send). Hope this is OK and hope there is nothing big
missing anymore (or broken).

Sorry about the delay. I've been really busy with some internal tasks.
I'm guessing we missed v4.8 with this thing. I'm sorry about that.

I'm including in this series a driver for the Broxton PMIC USB Type-C
PHY.


Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.


Heikki Krogerus (2):
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  163 
 Documentation/usb/typec.txt |  101 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |   21 +
 drivers/usb/typec/Makefile  |2 +
 drivers/usb/typec/typec.c   | 1173 +++
 drivers/usb/typec/typec_wcove.c |  376 +
 include/linux/usb/typec.h   |  255 ++
 10 files changed, 2104 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when USB PD is
available, also control over power role swapping and
Alternate Modes.

Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  163 
 Documentation/usb/typec.txt |  101 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |7 +
 drivers/usb/typec/Makefile  |1 +
 drivers/usb/typec/typec.c   | 1173 +++
 include/linux/usb/typec.h   |  255 ++
 9 files changed, 1713 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index 000..ce577f3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,163 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current USB data role the port is operating in. This
+   attribute can be used for requestion data role swapping on th
+   port.
+
+What:  /sys/class/typec//current_power_role
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current power role, source or sink, of the port. This
+   attribute can be used to request power role swap on the port
+   when USB Power Delivery is available.
+
+What:  /sys/class/typec//current_vconn_role
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current VCONN role, source or sink, of the port. This
+   attribute can be used to request vconn role swap on the port
+   when USB Power Delivery is available.
+
+What:  /sys/class/typec//power_operation_mode
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current power power mode the port is in. Reads as on
+   of the following:
+   - USB - Normal power levels defined in USB specifications
+   - BC1.2 - Power levels defined in Battery Charging Specification
+ v1.2
+   - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
+   specification.
+   - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
+   specification.
+- USB Power Delivery - The voltages and currents defined in USB
+  Power Delivery specification
+
+What:  /sys/class/typec//preferred_role
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   The user space can notify the driver about the preferred role.
+   It should be handled as enabling of Try.SRC or Try.SNK, as
+   defined in USB Type-C specification, in the port drivers.
+
+What:  /sys/class/typec//supported_accessory_modes
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Lists the Accessory Modes, defined in the USB Type-C
+   specification, the port supports.
+
+What:  /sys/class/typec//supported_data_roles
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Lists the USB data roles, host or device, the port is capable
+   of supporting.
+
+What:  /sys/class/typec//supported_power_roles
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Lists the power roles, source and/or sink, the port is capable
+   of supporting.
+
+What:  /sys/class/typec//supports_usb_power_delivery
+Data:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows if the port support USB Power Delivery.
+
+
+USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
+
+What:  

[PATCHv3 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

[Heikki Krogerus]

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus 
---
 drivers/usb/typec/Kconfig   |  14 ++
 drivers/usb/typec/Makefile  |   1 +
 drivers/usb/typec/typec_wcove.c | 376 
 3 files changed, 391 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index b229fb9..7a345a4 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB PD and Type-C drivers"
 config TYPEC
tristate
 
+config TYPEC_WCOVE
+   tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+   depends on ACPI
+   depends on INTEL_SOC_PMIC
+   depends on INTEL_PMC_IPC
+   select TYPEC
+   help
+ This driver adds support for USB Type-C detection on Intel Broxton
+ platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+ role and cable orientation.
+
+ To compile this driver as module, choose M here: the module will be
+ called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8b..b9cb862 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index 000..4309ac6
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,376 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ * Author: Heikki Krogerus 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0 0x4e09
+#define WCOVE_PHYCTRL  0x5e07
+
+#define USBC_CONTROL1  0x7001
+#define USBC_CONTROL2  0x7002
+#define USBC_CONTROL3  0x7003
+#define USBC_CC1_CTRL  0x7004
+#define USBC_CC2_CTRL  0x7005
+#define USBC_STATUS1   0x7007
+#define USBC_STATUS2   0x7008
+#define USBC_STATUS3   0x7009
+#define USBC_IRQ1  0x7015
+#define USBC_IRQ2  0x7016
+#define USBC_IRQMASK1  0x7017
+#define USBC_IRQMASK2  0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)  ((r & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNKBIT(0)
+#define USBC_CONTROL2_UNATT_SRCBIT(1)
+#define USBC_CONTROL2_DIS_ST   BIT(2)
+
+#define USBC_CONTROL3_PD_DIS   BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN  BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING   BIT(6)
+#define USBC_STATUS1_RSLT(r)   (r & 0xf)
+#define USBC_RSLT_NOTHING  0
+#define USBC_RSLT_SRC_DEFAULT  1
+#define USBC_RSLT_SRC_1_5A 2
+#define USBC_RSLT_SRC_3_0A 3
+#define USBC_RSLT_SNK  4
+#define USBC_RSLT_DEBUG_ACC5
+#define USBC_RSLT_AUDIO_ACC6
+#define USBC_RSLT_UNDEF15
+#define USBC_STATUS1_ORIENT(r) ((r >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL 1
+#define USBC_ORIENT_REVERSE2
+
+#define USBC_STATUS2_VBUS_REQ  BIT(5)
+
+#define USBC_IRQ1_ADCDONE1 BIT(2)
+#define USBC_IRQ1_OVERTEMP BIT(1)
+#define USBC_IRQ1_SHORTBIT(0)
+
+#define USBC_IRQ2_CC_CHANGEBIT(7)
+#define USBC_IRQ2_RX_PDBIT(6)
+#define USBC_IRQ2_RX_HRBIT(5)
+#define USBC_IRQ2_RX_CRBIT(4)
+#define USBC_IRQ2_TX_SUCCESS   BIT(3)
+#define USBC_IRQ2_TX_FAIL  BIT(2)
+
+#define USBC_IRQMASK1_ALL  (USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL  (USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+   struct mutex lock; /* device lock */
+   struct device *dev;
+   struct regmap *regmap;
+   struct typec_port *port;
+   struct completion complete;
+   struct typec_capability cap;
+   struct typec_connection con;
+   struct typec_partner partner;
+};
+
+enum wcove_typec_func {
+   WCOVE_FUNC_DRIVE_VBUS = 1,
+   WCOVE_FUNC_ORIENTATION,
+   WCOVE_FUNC_ROLE,
+   WCOVE_FUNC_DRIVE_VCONN,
+};
+
+enum wcove_typec_orientation {
+   

Re: [PATCH v12 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> This patch introduces the usb charger driver based on usb gadget that
>>> makes an enhancement to a power driver. It works well in practice but
>>> that requires a system with suitable hardware.
>>>
>>> The basic conception of the usb charger is that, when one usb charger
>>> is added or removed by reporting from the usb gadget state change or
>>> the extcon device state change, the usb charger will report to power
>>> user to set the current limitation.
>>>
>>> The usb charger will register notifiees on the usb gadget or the extcon
>>> device to get notified the usb charger state. It also supplies the
>>> notification mechanism to userspace When the usb charger state is changed.
>>>
>>> Power user will register a notifiee on the usb charger to get notified
>>> by status changes from the usb charger. It will report to power user
>>> to set the current limitation when detecting the usb charger is added
>>> or removed from extcon device state or usb gadget state.
>>>
>>> This patch doesn't yet integrate with the gadget code, so some functions
>>> which rely on the 'gadget' are not completed, that will be implemented
>>> in the following patches.
>>>
>>> Signed-off-by: Baolin Wang 
>>> Reviewed-by: Li Jun 
>>> Tested-by: Li Jun 
>>> ---
>>>  drivers/usb/gadget/Kconfig   |7 +
>>>  drivers/usb/gadget/udc/Makefile  |1 +
>>>  drivers/usb/gadget/udc/charger.c |  770 
>>> ++
>>>  include/linux/usb/charger.h  |  191 ++
>>>  include/uapi/linux/usb/charger.h |   31 ++
>>>  5 files changed, 1000 insertions(+)
>>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>>  create mode 100644 include/linux/usb/charger.h
>>>  create mode 100644 include/uapi/linux/usb/charger.h
>>>
>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>> index 2057add..89f4e9b 100644
>>> --- a/drivers/usb/gadget/Kconfig
>>> +++ b/drivers/usb/gadget/Kconfig
>>> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>>>   help
>>>  It supports the serial gadget can be used as a console.
>>>
>>> +config USB_CHARGER
>>> + bool "USB charger support"
>>
>> you didn't build test all possibilities, did you?
>>
>> I have a feeling this won't link if USB_GADGET=m. Can you test that?
>
> OK.
>
>>
>>> diff --git a/drivers/usb/gadget/udc/charger.c 
>>> b/drivers/usb/gadget/udc/charger.c
>> [...]
>>> +struct class *usb_charger_class;
>>
>> We already have a UDC class, do we really, really need another class
>> here?
>
> We want to manage the usb charger devices by this class and one usb
> charger device is not equal with one usb device which managed by UDC

Can you explain this statement? If charging is done via a USB peripheral
port, why don't we have a 1:1 mapping between charger and UDC?

> class, so can we use UDC class to manage charger devices?
> By the way, you also suggested to use the 'class' things instead of
> 'bus' in previous mail.

true, I did say that. It seems clearer, however, that we don't need this
virtual device here.

>>> +subsys_initcall(usb_charger_class_init);
>>
>> this should always work as module_init(). Please make sure that's the
>> case.
>
> we should make sure the charger class has been allocated before user
> try to add a new gadget to the udc class. So it should be issued
> before 'module_init()' (many usb drivers are at module_init level).

-EPROBE_DEFER?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with 
>> >> >> >> >>> OTG core.
>> >> >> >> >> 
>> >> >> >> >> do you really know of any platform which has a separate OTG 
>> >> >> >> >> controller?
>> >> >> >> >> 
>> >> >> >> >
>> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks 
>> >> >> >> > for OTG, host
>> >> >> >> > and gadget.
>> >> >> >> >
>> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> >> >> 
>> >> >> >> that's not an OTG controller, it's just a mux. No different than 
>> >> >> >> Intel's
>> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> >> >> 
>> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. 
>> >> >> >> They
>> >> >> >> are two competing standards and, apparently, type-C is winning when 
>> >> >> >> it
>> >> >> >> comes to role-swapping.
>> >> >> >> 
>> >> >> >
>> >> >> > In fact, OTG is mis-used by people. Currently, if the port is 
>> >> >> > dual-role,
>> >> >> > It will be considered as an OTG port.
>> >> >> 
>> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
>> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally 
>> >> >> referring
>> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>> >> >
>> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> >> > "OTG" in Linux, right?
>> >> 
>> >> to avoid confusion with the terminology, yes. With that settled, let's
>> >> figure out how you can deliver what your marketting guys are asking of
>> >> you.
>> >> 
>> >
>> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
>> > test. The internal bsp has passed PET test, and formal compliance test
>> > is on the way (should pass too). 
>> >
>> > The dual-role and OTG compliance use the same zImage, but different
>> > dtb.
>> 
>> okay, that's good to know. Now, the question really is: considering we
>> only have one user for this generic OTG FSM layer, do we really need to
>> make it generic at all? I mean, just look at how invasive a change that
>> is.
>
> If the chipidea is the only user for this roger's framework, I don't
> think it is necessary. In fact, Roger introduces this framework, and
> the first user is dwc3, we think it can be used for others. Let's

Right, we need to look at the history of dwc3 to figure out why the
conclusion that dwc3 needs this was made.

Roger started working on this framework when Power on Reset section of
databook had some details which weren't always clear and, for safety, we
always had reset asserted for a really long time. It was so long (about
400 ms) that resetting dwc3 for each role swap was just too much.

Coupled with that, the OTG chapter wasn't very clear either on
expections from Host and Peripheral side initialization in OTG/DRD
systems.

More recent version of dwc3 databook have a much better description of
how and which reset bits _must_ be asserted and which shouldn't be
touched unless it's for debugging purposes. When I implemented that, our
->probe() went from 400ms down to about 50us.

Coupled with that, the OTG chapter also became a lot clearer to the
point that it states you just don't initialize anything other than the
OTG block, and just wait for OTG interrupt to do whatever it is you need
to do.

This meant that we could actually afford to do full reinitialization of
dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
roles properly.

(The reason for needing soft-reset during role swap is kinda long. But
in summary dwc3 shadows register writes to both host and peripheral
sides)

> just discuss if it is necessary for dual-role switch.

fair. However, if we have a single user we don't have a Generic
layer. There's not enough variance to come up with truly generic
architecture for this.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>
> Yeah, I also hope so. But we really want something to manage the
> charger devices, do you have any good suggestion to avoid the 'class'
> but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real
devices, just a handle as part of struct usb_gadget, no?

-- 
balbi


signature.asc
Description: PGP signature

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Tue, Jun 21, 2016 at 03:08:52PM +0200, Oliver Neukum wrote:
> On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > The purpose of this class is to provide unified interface for user
> > space to get the status and basic information about USB Type-C
> > Connectors in the system, control data role swapping, and when USB PD
> > is available, also power role swapping and Alternate Modes.
> 
> This raises two more questions.
> 
> 1. Booting
> 
> It is possible that our only display and, worse, our source
> of power is a display that can be used only in an alternate mode
> and is connected via a type C connector.
> 
> We need some kind of boot time support for alternate modes.
> 
> The firmware will surely want to display something. So it is possible
> that we start the OS will a valid power contract. How do we deal
> with that? Renegotiate?

Systems where the firmware has to negotiate PD will likely provide
firmware interface like UCSI, and where the OS has no direct
interaction with the USB PD transceiver. In these case there is no
need to renegotiate as we are just reporting in OS the initial state
after bootup.

We do have a system where the typec port is used to power the board.
On these systems the firmware does not communicate PD (so we will
never have the firmware displaying anything over Type-C on those
systems), but the USB PD chargers for example are detected as 3.0A
Type-C power supplies before any USB PD negotiation takes place, just
like the spec says, and that is more then enough to power these boards.

> 2. Multiple GPUs
> 
> How do we know which GPU is connected to which port?

With ACPI we will have to be able to bind the correct companion to the
device presenting the DP alternate mode under the port. Just like we
will have to bind the ACPI companion of the actual USB port to the
typec port device itself.


Cheers,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface for user
> space to get the status and basic information about USB Type-C
> Connectors in the system, control data role swapping, and when USB PD
> is available, also power role swapping and Alternate Modes.

This raises two more questions.

1. Booting

It is possible that our only display and, worse, our source
of power is a display that can be used only in an alternate mode
and is connected via a type C connector.

We need some kind of boot time support for alternate modes.

The firmware will surely want to display something. So it is possible
that we start the OS will a valid power contract. How do we deal
with that? Renegotiate?

2. Multiple GPUs

How do we know which GPU is connected to which port?

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 03:35:00PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with 
> >> >> >> >>> OTG core.
> >> >> >> >> 
> >> >> >> >> do you really know of any platform which has a separate OTG 
> >> >> >> >> controller?
> >> >> >> >> 
> >> >> >> >
> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks 
> >> >> >> > for OTG, host
> >> >> >> > and gadget.
> >> >> >> >
> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> >> 
> >> >> >> that's not an OTG controller, it's just a mux. No different than 
> >> >> >> Intel's
> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> >> 
> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. 
> >> >> >> They
> >> >> >> are two competing standards and, apparently, type-C is winning when 
> >> >> >> it
> >> >> >> comes to role-swapping.
> >> >> >> 
> >> >> >
> >> >> > In fact, OTG is mis-used by people. Currently, if the port is 
> >> >> > dual-role,
> >> >> > It will be considered as an OTG port.
> >> >> 
> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >> >
> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> > "OTG" in Linux, right?
> >> 
> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> figure out how you can deliver what your marketting guys are asking of
> >> you.
> >> 
> >
> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> > test. The internal bsp has passed PET test, and formal compliance test
> > is on the way (should pass too). 
> >
> > The dual-role and OTG compliance use the same zImage, but different
> > dtb.
> 
> okay, that's good to know. Now, the question really is: considering we
> only have one user for this generic OTG FSM layer, do we really need to
> make it generic at all? I mean, just look at how invasive a change that
> is.
> 

If the chipidea is the only user for this roger's framework, I don't
think it is necessary. In fact, Roger introduces this framework, and
the first user is dwc3, we think it can be used for others. Let's
just discuss if it is necessary for dual-role switch.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Guenter Roeck]

On 06/21/2016 06:08 AM, Oliver Neukum wrote:

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:

The purpose of this class is to provide unified interface for user
space to get the status and basic information about USB Type-C
Connectors in the system, control data role swapping, and when USB PD
is available, also power role swapping and Alternate Modes.


This raises two more questions.

1. Booting

It is possible that our only display and, worse, our source
of power is a display that can be used only in an alternate mode
and is connected via a type C connector.

We need some kind of boot time support for alternate modes.

The firmware will surely want to display something. So it is possible
that we start the OS will a valid power contract. How do we deal
with that? Renegotiate?



In one of my drivers, the PD protocol is running on an EC and the Linux
driver is just interfacing it to the typec class. I don't do any renegotiating
but just report the port state to the class. What is wrong with that, and why
would it not work ?

If the PD protocol runs in Linux (which I implement as well), I tried 
renegotiating,
ie assuming that a contract was already established, with several multi-function
adapters. Quite often those 'die' when trying tricks like that, and have to be 
manually
disconnected from power (on both ends) to get back to life. Granted, part of 
that
is that the firmware on those adapters is early and not as stable as I would 
like it
to be, but still that is a significant risk. I ended up always starting with 
error
recovery to avoid that kind of problem, at least for now. Even that doesn't 
always
help.

> 2. Multiple GPUs


How do we know which GPU is connected to which port?



Still working on it, but the basic idea (in my case) is to use devicetree data
or platform data if that is not available.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

 We can get/put usb charger and mange usb charger attributes with the
 device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
  ^
   we want   

> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.
>
> -- 
> balbi

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v12 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

On 21 June 2016 at 18:25, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> This patch introduces the usb charger driver based on usb gadget that
>> makes an enhancement to a power driver. It works well in practice but
>> that requires a system with suitable hardware.
>>
>> The basic conception of the usb charger is that, when one usb charger
>> is added or removed by reporting from the usb gadget state change or
>> the extcon device state change, the usb charger will report to power
>> user to set the current limitation.
>>
>> The usb charger will register notifiees on the usb gadget or the extcon
>> device to get notified the usb charger state. It also supplies the
>> notification mechanism to userspace When the usb charger state is changed.
>>
>> Power user will register a notifiee on the usb charger to get notified
>> by status changes from the usb charger. It will report to power user
>> to set the current limitation when detecting the usb charger is added
>> or removed from extcon device state or usb gadget state.
>>
>> This patch doesn't yet integrate with the gadget code, so some functions
>> which rely on the 'gadget' are not completed, that will be implemented
>> in the following patches.
>>
>> Signed-off-by: Baolin Wang 
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>> ---
>>  drivers/usb/gadget/Kconfig   |7 +
>>  drivers/usb/gadget/udc/Makefile  |1 +
>>  drivers/usb/gadget/udc/charger.c |  770 
>> ++
>>  include/linux/usb/charger.h  |  191 ++
>>  include/uapi/linux/usb/charger.h |   31 ++
>>  5 files changed, 1000 insertions(+)
>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>  create mode 100644 include/linux/usb/charger.h
>>  create mode 100644 include/uapi/linux/usb/charger.h
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 2057add..89f4e9b 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>>   help
>>  It supports the serial gadget can be used as a console.
>>
>> +config USB_CHARGER
>> + bool "USB charger support"
>
> you didn't build test all possibilities, did you?
>
> I have a feeling this won't link if USB_GADGET=m. Can you test that?

OK.

>
>> diff --git a/drivers/usb/gadget/udc/charger.c 
>> b/drivers/usb/gadget/udc/charger.c
> [...]
>> +struct class *usb_charger_class;
>
> We already have a UDC class, do we really, really need another class
> here?

We want to manage the usb charger devices by this class and one usb
charger device is not equal with one usb device which managed by UDC
class, so can we use UDC class to manage charger devices?
By the way, you also suggested to use the 'class' things instead of
'bus' in previous mail.

>
>> +subsys_initcall(usb_charger_class_init);
>
> this should always work as module_init(). Please make sure that's the
> case.

we should make sure the charger class has been allocated before user
try to add a new gadget to the udc class. So it should be issued
before 'module_init()' (many usb drivers are at module_init level).
Another hand this follows the UDC class allocation.

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
 Can't you just tie a charger to a UDC and avoid the charger class
 completely?
>>>
>>> Yeah, I also hope so. But we really want something to manage the
>>> charger devices, do you have any good suggestion to avoid the 'class'
>>> but also can manage the charger devices?
>>
>> manage in what way? It seems to me that they don't need to be real
>> devices, just a handle as part of struct usb_gadget, no?
>
> Although charger device is not one real hardware device, we also use
> one 'struct device' to describe it in charger.c file. So we should
> manage the 'struct device' with one proper way.

 that's fine, but why do you think they need a struct device to start with?
>>>
>>> We can get/put usb charger and mange usb charger attributes with the
>>> device model if we use a struct device.
>>
>> We already have that as part of struct usb_udc. Why don't you just
>> create a subdirectory called charger which will hold all your
>> charger-related attributes. That directory will only be created if a
>> valid ->charger pointer exists.
>
> That means we can remove all the device and class things in charger.c
> file, right? OK, I try to do that. Thanks.

right. Keep your charger.c file, because to conditionally compile and
link that to udc-core.ko, but remove all the class initialization and
all of that extra code.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 20:36, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
> Baolin Wang  writes:
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>
>> Yeah, I also hope so. But we really want something to manage the
>> charger devices, do you have any good suggestion to avoid the 'class'
>> but also can manage the charger devices?
>
> manage in what way? It seems to me that they don't need to be real
> devices, just a handle as part of struct usb_gadget, no?

 Although charger device is not one real hardware device, we also use
 one 'struct device' to describe it in charger.c file. So we should
 manage the 'struct device' with one proper way.
>>>
>>> that's fine, but why do you think they need a struct device to start with?
>>
>> We can get/put usb charger and mange usb charger attributes with the
>> device model if we use a struct device.
>
> We already have that as part of struct usb_udc. Why don't you just
> create a subdirectory called charger which will hold all your
> charger-related attributes. That directory will only be created if a
> valid ->charger pointer exists.

That means we can remove all the device and class things in charger.c
file, right? OK, I try to do that. Thanks.

>
> USB Charging is always tied to a peripheral side controller, anyway.
>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
 Can't you just tie a charger to a UDC and avoid the charger class
 completely?
>>>
>>> Yeah, I also hope so. But we really want something to manage the
>>> charger devices, do you have any good suggestion to avoid the 'class'
>>> but also can manage the charger devices?
>>
>> manage in what way? It seems to me that they don't need to be real
>> devices, just a handle as part of struct usb_gadget, no?
>
> Although charger device is not one real hardware device, we also use
> one 'struct device' to describe it in charger.c file. So we should
> manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
 Baolin Wang  writes:
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>
> Yeah, I also hope so. But we really want something to manage the
> charger devices, do you have any good suggestion to avoid the 'class'
> but also can manage the charger devices?

 manage in what way? It seems to me that they don't need to be real
 devices, just a handle as part of struct usb_gadget, no?
>>>
>>> Although charger device is not one real hardware device, we also use
>>> one 'struct device' to describe it in charger.c file. So we should
>>> manage the 'struct device' with one proper way.
>>
>> that's fine, but why do you think they need a struct device to start with?
>
> We can get/put usb charger and mange usb charger attributes with the
> device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just
create a subdirectory called charger which will hold all your
charger-related attributes. That directory will only be created if a
valid ->charger pointer exists.

USB Charging is always tied to a peripheral side controller, anyway.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG 
>> >> >> >>> core.
>> >> >> >> 
>> >> >> >> do you really know of any platform which has a separate OTG 
>> >> >> >> controller?
>> >> >> >> 
>> >> >> >
>> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for 
>> >> >> > OTG, host
>> >> >> > and gadget.
>> >> >> >
>> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> >> 
>> >> >> that's not an OTG controller, it's just a mux. No different than 
>> >> >> Intel's
>> >> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> >> 
>> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> >> >> are two competing standards and, apparently, type-C is winning when it
>> >> >> comes to role-swapping.
>> >> >> 
>> >> >
>> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
>> >> > It will be considered as an OTG port.
>> >> 
>> >> That's because "dual-role" is a non-standard OTG. Seen as people really
>> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
>> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>> >
>> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> > "OTG" in Linux, right?
>> 
>> to avoid confusion with the terminology, yes. With that settled, let's
>> figure out how you can deliver what your marketting guys are asking of
>> you.
>> 
>
> Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> test. The internal bsp has passed PET test, and formal compliance test
> is on the way (should pass too). 
>
> The dual-role and OTG compliance use the same zImage, but different
> dtb.

okay, that's good to know. Now, the question really is: considering we
only have one user for this generic OTG FSM layer, do we really need to
make it generic at all? I mean, just look at how invasive a change that
is.

My fear is that, as stated before, we don't have enough variance to be
able to design something that many could use. On top of that, most folks
are moving to type-c connector which, in reality, can't really implement
OTG.

Considering that Apple/Intel have already announced [1] that they will
use type-c connector, it's not too farfetched to speculate that CarPlay
will, eventually, rely on Power Delivery for role swapping. IOW, OTG has
its days counted. In 2 years' time, the market will have moved on to
Type-C and the generic OTG layer will be left to bit rot as time goes
by.

This is why I think that these changes should be local to chipidea,
considering chipidea is the only user for them. As for dwc3, we can get
something much simpler since, at least so far, there's no full OTG
requirement from anywhere I know and, even if OTG becomes a requirement
for any of dwc3 users, the HW handles the state machine for us.

What do you think?

[1] https://thunderbolttechnology.net/blog/thunderbolt-3-usb-c-does-it-all

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 20:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the
device model if we use a struct device.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> On 21 June 2016 at 19:03, Mark Brown  wrote:
>> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>>> Baolin Wang  writes:
>>> > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
>>> > *pdev)
>>> > }
>>> > }
>>
>>> > +   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
>>> > +   power->usb_charger =
>>> > +   usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>>
>>> the fact that you rely on strings and pass them via pdata is an
>>> indication that you don't have enough description of the HW. Seems like
>>> we need to come up with a set of DT properties which tie a charger to a
>>> UDC.
>>
>>> I'm thinking a phandle would be enough?
>>
>> The wm831x has no DT support currently.
>
> Another hand I suppose the usb charger is one virtual device and does
> not need be described from DT.

Right, I don't think that should be a device at all. But you can pass a
phandle to the UDC controller and use that to get to struct usb_gadget
from which you could reach ->charger.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 14/14] xhci: get rid of platform data

[Mathias Nyman]

From: Heikki Krogerus 

No more users for it.

Signed-off-by: Heikki Krogerus 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-plat.c |  5 +
 include/linux/usb/xhci_pdriver.h | 27 ---
 2 files changed, 1 insertion(+), 31 deletions(-)
 delete mode 100644 include/linux/usb/xhci_pdriver.h

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ff916b3..906e445 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "xhci.h"
@@ -138,7 +137,6 @@ MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 
 static int xhci_plat_probe(struct platform_device *pdev)
 {
-   struct usb_xhci_pdata   *pdata = dev_get_platdata(>dev);
const struct of_device_id *match;
const struct hc_driver  *driver;
struct xhci_hcd *xhci;
@@ -219,8 +217,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
goto disable_clk;
}
 
-   if (device_property_read_bool(>dev, "usb3-lpm-capable") ||
-   (pdata && pdata->usb3_lpm_capable))
+   if (device_property_read_bool(>dev, "usb3-lpm-capable"))
xhci->quirks |= XHCI_LPM_SUPPORT;
 
if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
diff --git a/include/linux/usb/xhci_pdriver.h b/include/linux/usb/xhci_pdriver.h
deleted file mode 100644
index 376654b..000
--- a/include/linux/usb/xhci_pdriver.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
- * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
- * for more details.
- *
- */
-
-#ifndef __USB_CORE_XHCI_PDRIVER_H
-#define __USB_CORE_XHCI_PDRIVER_H
-
-/**
- * struct usb_xhci_pdata - platform_data for generic xhci platform driver
- *
- * @usb3_lpm_capable:  determines if this xhci platform supports USB3
- * LPM capability
- *
- */
-struct usb_xhci_pdata {
-   unsignedusb3_lpm_capable:1;
-};
-
-#endif /* __USB_CORE_XHCI_PDRIVER_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 19:49, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>
>> Yeah, I also hope so. But we really want something to manage the
>> charger devices, do you have any good suggestion to avoid the 'class'
>> but also can manage the charger devices?
>
> manage in what way? It seems to me that they don't need to be real
> devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use
one 'struct device' to describe it in charger.c file. So we should
manage the 'struct device' with one proper way.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

On 21 June 2016 at 19:53, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> On 21 June 2016 at 19:03, Mark Brown  wrote:
>>> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
 Baolin Wang  writes:
 > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct 
 > platform_device *pdev)
 > }
 > }
>>>
 > +   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
 > +   power->usb_charger =
 > +   usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>>>
 the fact that you rely on strings and pass them via pdata is an
 indication that you don't have enough description of the HW. Seems like
 we need to come up with a set of DT properties which tie a charger to a
 UDC.
>>>
 I'm thinking a phandle would be enough?
>>>
>>> The wm831x has no DT support currently.
>>
>> Another hand I suppose the usb charger is one virtual device and does
>> not need be described from DT.
>
> Right, I don't think that should be a device at all. But you can pass a
> phandle to the UDC controller and use that to get to struct usb_gadget
> from which you could reach ->charger.

Ah, make sense.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

Hi,

Mark Brown  writes:
> [ Unknown signature status ]
> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>> Baolin Wang  writes:
>> > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
>> > *pdev)
>> >}
>> >}
>
>> > +  if (wm831x_pdata && wm831x_pdata->usb_gadget) {
>> > +  power->usb_charger =
>> > +  usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>
>> the fact that you rely on strings and pass them via pdata is an
>> indication that you don't have enough description of the HW. Seems like
>> we need to come up with a set of DT properties which tie a charger to a
>> UDC.
>
>> I'm thinking a phandle would be enough?
>
> The wm831x has no DT support currently.

okay, perhaps its time to add it.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 11:18:21AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> >> Peter Chen  writes:
> >> >> >>> +
> >> >> >>> +   /* start host */
> >> >> >>> +   ret = hcd_ops->add(otg->primary_hcd.hcd,
> >> >> >>> +  otg->primary_hcd.irqnum,
> >> >> >>> +  otg->primary_hcd.irqflags);
> >> >> >> 
> >> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
> >> >> >
> >> >> > I've introduced the host and gadget ops interface to get around the
> >> >> > circular dependency issue we can't avoid.
> >> >> > otg needs to call host/gadget functions and host/gadget also needs to
> >> >> > call otg functions.
> >> >> 
> >> >> IMO, this shows a fragility of your design. You're, now, lying to
> >> >> usb_hcd and usb_udc and making them register into a virtual layer that
> >> >> doesn't exist. And that layer will end up calling the real registration
> >> >> function when some magic event happens.
> >> >> 
> >> >> This is only really needed for quirky devices like dwc3 (but see more on
> >> >> dwc3 below) where host and peripheral registers shadow each
> >> >> other. Otherwise we would be able to always keep hcd and udc always
> >> >> registered. They would get different interrupt statuses anyway and
> >> >> nothing would ever break.
> >> >> 
> >> >> However, when it comes to dwc3, we already have all the code necessary
> >> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> >> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> >> >> the OTG states for those folks who really care about OTG (Hint: nobody
> >> >> has cared for the past 10 years, why would they do so now?) and we don't
> >> >> need a SW state machine when the HW handles that for us, right?
> >> >> 
> >> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> >> >> I know very little about that IP and don't even have documentation on
> >> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> >> >> which changes roles automatically in HW based on ID pin state.
> >> >
> >> > Chipidea needs to set register for USB role manually.
> >> 
> >> okay, so chipidea has private control of role. Much like dwc3. That's good.
> >> 
> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG 
> >> >> >>> core.
> >> >> >> 
> >> >> >> do you really know of any platform which has a separate OTG 
> >> >> >> controller?
> >> >> >> 
> >> >> >
> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for 
> >> >> > OTG, host
> >> >> > and gadget.
> >> >> >
> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> 
> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> 
> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> >> are two competing standards and, apparently, type-C is winning when it
> >> >> comes to role-swapping.
> >> >> 
> >> >
> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> >> > It will be considered as an OTG port.
> >> 
> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >
> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> > "OTG" in Linux, right?
> 
> to avoid confusion with the terminology, yes. With that settled, let's
> figure out how you can deliver what your marketting guys are asking of
> you.
> 

Since nxp SoC claims they are OTG compliance, we need to pass usb.org
test. The internal bsp has passed PET test, and formal compliance test
is on the way (should pass too). 

The dual-role and OTG compliance use the same zImage, but different
dtb.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

On 21 June 2016 at 19:03, Mark Brown  wrote:
> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>> Baolin Wang  writes:
>> > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
>> > *pdev)
>> > }
>> > }
>
>> > +   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
>> > +   power->usb_charger =
>> > +   usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>
>> the fact that you rely on strings and pass them via pdata is an
>> indication that you don't have enough description of the HW. Seems like
>> we need to come up with a set of DT properties which tie a charger to a
>> UDC.
>
>> I'm thinking a phandle would be enough?
>
> The wm831x has no DT support currently.

Another hand I suppose the usb charger is one virtual device and does
not need be described from DT.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
> Baolin Wang  writes:
> > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
> > *pdev)
> > }
> > }

> > +   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> > +   power->usb_charger =
> > +   usb_charger_find_by_name(wm831x_pdata->usb_gadget);

> the fact that you rely on strings and pass them via pdata is an
> indication that you don't have enough description of the HW. Seems like
> we need to come up with a set of DT properties which tie a charger to a
> UDC.

> I'm thinking a phandle would be enough?

The wm831x has no DT support currently.


signature.asc
Description: PGP signature

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 18:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> For supporting the usb charger, it adds the usb_charger_init() and
>> usb_charger_exit() functions for usb charger initialization and exit.
>>
>> It will report to the usb charger when the gadget state is changed,
>> then the usb charger can do the power things.
>>
>> Signed-off-by: Baolin Wang 
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>
> Before anything, I must say that I really liked this patch. It's

Thanks.

> minimaly invasive to udc core and does all the necessary changes. If it
> wasn't for the extra charger class, this would've been perfect.
>
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

Yeah, I also hope so. But we really want something to manage the
charger devices, do you have any good suggestion to avoid the 'class'
but also can manage the charger devices?

>
>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned 
>> mA)
>>  {
>> + if (gadget->charger)
>
> I guess you could do this check inside
> usb_gadget_set_cur_limit_by_type() itself.

OK.

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> For supporting the usb charger, it adds the usb_charger_init() and
> usb_charger_exit() functions for usb charger initialization and exit.
>
> It will report to the usb charger when the gadget state is changed,
> then the usb charger can do the power things.
>
> Signed-off-by: Baolin Wang 
> Reviewed-by: Li Jun 
> Tested-by: Li Jun 

Before anything, I must say that I really liked this patch. It's
minimaly invasive to udc core and does all the necessary changes. If it
wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class
completely?

>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned 
> mA)
>  {
> + if (gadget->charger)

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 12/14] xhci: plat: adapt to unified device property interface

[Mathias Nyman]

From: Heikki Krogerus 

Requesting the only property that the driver needs using the
unified device property interface so it will be available
for all types of platforms, not just the ones using DT.

Signed-off-by: Heikki Krogerus 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-plat.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 676ea45..ff916b3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -138,7 +138,6 @@ MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 
 static int xhci_plat_probe(struct platform_device *pdev)
 {
-   struct device_node  *node = pdev->dev.of_node;
struct usb_xhci_pdata   *pdata = dev_get_platdata(>dev);
const struct of_device_id *match;
const struct hc_driver  *driver;
@@ -199,7 +198,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
}
 
xhci = hcd_to_xhci(hcd);
-   match = of_match_node(usb_xhci_of_match, node);
+   match = of_match_node(usb_xhci_of_match, pdev->dev.of_node);
if (match) {
const struct xhci_plat_priv *priv_match = match->data;
struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
@@ -220,7 +219,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
goto disable_clk;
}
 
-   if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
+   if (device_property_read_bool(>dev, "usb3-lpm-capable") ||
(pdata && pdata->usb3_lpm_capable))
xhci->quirks |= XHCI_LPM_SUPPORT;
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Tony Lindgren  writes:
> * Felipe Balbi  [160621 03:06]:
>> 8<--
>>  vbus = read(VBUS_STATE); /* could be a gpio_get_value() */
>> id = read(ID_STATE); /* could be a gpio_get_value() */
>> 
>> set_role(id);
>> set_vbus(vbus);
>
> We should use regulator framework API for set_vbus() because of
> the delays involved bringing it up. And we already have separate
> PHY and charger chips where VBUS is provided by the charger chip.

yeah, no arguments there.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Tony Lindgren]

* Felipe Balbi  [160621 03:06]:
> 8<--
>   vbus = read(VBUS_STATE); /* could be a gpio_get_value() */
> id = read(ID_STATE); /* could be a gpio_get_value() */
> 
> set_role(id);
> set_vbus(vbus);

We should use regulator framework API for set_vbus() because of
the delays involved bringing it up. And we already have separate
PHY and charger chips where VBUS is provided by the charger chip.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Frank Wang]

The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
than rk3288 and before, and most of phy-related registers are also
different from the past, so a new phy driver is required necessarily.

Signed-off-by: Frank Wang 
Suggested-by: Heiko Stuebner 
Suggested-by: Guenter Roeck 
Suggested-by: Doug Anderson 
---

Changes in v7:
 - renamed functions *usb2phy_resume/*usb2phy_suspend to 
*usb2phy_power_on/usb2phy_power_off.

Changes in v6:
 - Fixed the output clock would be disabled more than once while phy-port was 
going to suspend.
 - Improved the driver to avoid the currently empty otg-port would cause 
null-pointer dereferences.

Changes in v5:
 - Added 'reg' in the data block to match the different phy-blocks in dt.

Changes in v4:
 - Removed some processes related to 'vbus_host-supply'.

Changes in v3:
 - Resolved the mapping defect between fixed value in driver and the property
   in devicetree.
 - Optimized 480m output clock register function.
 - Code cleanup.

Changes in v2:
 - Changed vbus_host operation from gpio to regulator in *_probe.
 - Improved the fault treatment relate to 480m clock register.
 - Cleaned up some meaningless codes in *_clk480m_disable.
 - made more clear the comment of *_sm_work.

 drivers/phy/Kconfig  |7 +
 drivers/phy/Makefile |1 +
 drivers/phy/phy-rockchip-inno-usb2.c |  658 ++
 3 files changed, 666 insertions(+)
 create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index b869b98..29ef15c 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -347,6 +347,13 @@ config PHY_ROCKCHIP_USB
help
  Enable this to support the Rockchip USB 2.0 PHY.
 
+config PHY_ROCKCHIP_INNO_USB2
+   tristate "Rockchip INNO USB2PHY Driver"
+   depends on ARCH_ROCKCHIP && OF
+   select GENERIC_PHY
+   help
+ Support for Rockchip USB2.0 PHY with Innosilicon IP block.
+
 config PHY_ROCKCHIP_EMMC
tristate "Rockchip EMMC PHY Driver"
depends on ARCH_ROCKCHIP && OF
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9c3e73c..4963fbc 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -38,6 +38,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2)+= 
phy-s5pv210-usb2.o
 obj-$(CONFIG_PHY_EXYNOS5_USBDRD)   += phy-exynos5-usbdrd.o
 obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)+= phy-qcom-apq8064-sata.o
 obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
+obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2)   += phy-rockchip-inno-usb2.o
 obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
 obj-$(CONFIG_PHY_ROCKCHIP_DP)  += phy-rockchip-dp.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)+= phy-qcom-ipq806x-sata.o
diff --git a/drivers/phy/phy-rockchip-inno-usb2.c 
b/drivers/phy/phy-rockchip-inno-usb2.c
new file mode 100644
index 000..299f599
--- /dev/null
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -0,0 +1,658 @@
+/*
+ * Rockchip USB2.0 PHY with Innosilicon IP block driver
+ *
+ * Copyright (C) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BIT_WRITEABLE_SHIFT16
+#define SCHEDULE_DELAY (60 * HZ)
+
+enum rockchip_usb2phy_port_id {
+   USB2PHY_PORT_OTG,
+   USB2PHY_PORT_HOST,
+   USB2PHY_NUM_PORTS,
+};
+
+enum rockchip_usb2phy_host_state {
+   PHY_STATE_HS_ONLINE = 0,
+   PHY_STATE_DISCONNECT= 1,
+   PHY_STATE_HS_CONNECT= 2,
+   PHY_STATE_FS_CONNECT= 4,
+};
+
+struct usb2phy_reg {
+   unsigned intoffset;
+   unsigned intbitend;
+   unsigned intbitstart;
+   unsigned intdisable;
+   unsigned intenable;
+};
+
+/**
+ * struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
+ * @phy_sus: phy suspend register.
+ * @ls_det_en: linestate detection enable register.
+ * @ls_det_st: linestate detection state register.
+ * @ls_det_clr: linestate detection clear register.
+ * @utmi_ls: utmi linestate state register.
+ * @utmi_hstdet: utmi host disconnect register.
+ */
+struct rockchip_usb2phy_port_cfg {
+   struct usb2phy_reg  phy_sus;
+   struct usb2phy_reg  ls_det_en;
+   struct usb2phy_reg  

[PATCH] usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock

[iari]

From: Iago Abal 

Fixes: d3cb25a12138 (usb: gadget: udc: fix spin_lock in pch_udc)

The above commit reordered spin_lock/unlock and now `>lock' is acquired
(rather than released) before calling `dev->driver->disconnect',
`dev->driver->setup', `dev->driver->suspend', `usb_gadget_giveback_request',
and `usb_gadget_udc_reset'.

But this *may* not be the right way to fix the problem pointed by d3cb25a12138.

Note that the other usb/gadget/udc drivers do release the lock before calling
these functions. There are also inconsistencies within pch_udc.c, where
`dev->driver->disconnect' is called while holding `>lock' in lines 613 and
1184, but not in line 2739.

Finally, commit d3cb25a12138 may have introduced several potential deadlocks.
For instance, EBA (https://github.com/models-team/eba) reports:

Double lock in drivers/usb/gadget/udc/pch_udc.c
first at 2791: spin_lock(& dev->lock); [pch_udc_isr]
second at 2694: spin_lock(& dev->lock); [pch_udc_svc_cfg_interrupt]
after calling from 2793: pch_udc_dev_isr(dev, dev_intr);
after calling from 2724: pch_udc_svc_cfg_interrupt(dev);

Similarly, other potential deadlocks are 2791 -> 2793 -> 2721 -> 2657; and
2791 -> 2793 -> 2711 -> 2573 -> 1499 -> 1480.

Signed-off-by: Iago Abal 
---
 drivers/usb/gadget/udc/pch_udc.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index ebc51ec..7175142 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1477,11 +1477,11 @@ static void complete_req(struct pch_udc_ep *ep, struct 
pch_udc_request *req,
req->dma_mapped = 0;
}
ep->halted = 1;
-   spin_lock(>lock);
+   spin_unlock(>lock);
if (!ep->in)
pch_udc_ep_clear_rrdy(ep);
usb_gadget_giveback_request(>ep, >req);
-   spin_unlock(>lock);
+   spin_lock(>lock);
ep->halted = halted;
 }
 
@@ -2573,9 +2573,9 @@ static void pch_udc_svc_ur_interrupt(struct pch_udc_dev 
*dev)
empty_req_queue(ep);
}
if (dev->driver) {
-   spin_lock(>lock);
-   usb_gadget_udc_reset(>gadget, dev->driver);
spin_unlock(>lock);
+   usb_gadget_udc_reset(>gadget, dev->driver);
+   spin_lock(>lock);
}
 }
 
@@ -2654,9 +2654,9 @@ static void pch_udc_svc_intf_interrupt(struct pch_udc_dev 
*dev)
dev->ep[i].halted = 0;
}
dev->stall = 0;
-   spin_lock(>lock);
-   dev->driver->setup(>gadget, >setup_data);
spin_unlock(>lock);
+   dev->driver->setup(>gadget, >setup_data);
+   spin_lock(>lock);
 }
 
 /**
@@ -2691,9 +2691,9 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev 
*dev)
dev->stall = 0;
 
/* call gadget zero with setup data received */
-   spin_lock(>lock);
-   dev->driver->setup(>gadget, >setup_data);
spin_unlock(>lock);
+   dev->driver->setup(>gadget, >setup_data);
+   spin_lock(>lock);
 }
 
 /**
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 0/2] Add a new Rockchip usb2 phy driver

[Frank Wang]

The newer SoCs (rk3366, rk3399) of Rock-chip take a different usb-phy
IP block than rk3288 and before, and most of phy-related registers are
also different from the past, so a new phy driver is required necessarily.

These series patches add phy-rockchip-inno-usb2.c and the corresponding
documentation.

Changes in v7:
 - renamed functions *usb2phy_resume/*usb2phy_suspend to 
*usb2phy_power_on/usb2phy_power_off.

Changes in v6:
 - Changed '_' to '-' for otg-id and otg-bvalid property in devicetree bindings.
 - Fixed the output clock would be disabled more than once while phy-port was 
going to suspend.
 - Improved the driver to avoid the currently empty otg-port would cause 
null-pointer dereferences.

Changes in v5:
 - Added 'reg' property both in devicetree bindings and driver to match the 
different phy-blocks.

Changes in v4:
 - Used 'phy-supply' instead of 'vbus_*-supply'.

Changes in v3:
 - Supplemented some hardware-description into the devicetree bindings.
 - Resolved the mapping defect between fixed value in driver and the property
   in devicetree.
 - Code cleanup.

Changes in v2:
 - Specified more hardware-description into the devicetree bindings.
 - Optimized some process of driver.

Frank Wang (2):
  Documentation: bindings: add DT documentation for Rockchip USB2PHY
  phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

 .../bindings/phy/phy-rockchip-inno-usb2.txt|   64 ++
 drivers/phy/Kconfig|7 +
 drivers/phy/Makefile   |1 +
 drivers/phy/phy-rockchip-inno-usb2.c   |  658 
 4 files changed, 730 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
 create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c

-- 
1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
> *pdev)
>   }
>   }
>  
> + if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> + power->usb_charger =
> + usb_charger_find_by_name(wm831x_pdata->usb_gadget);

the fact that you rely on strings and pass them via pdata is an
indication that you don't have enough description of the HW. Seems like
we need to come up with a set of DT properties which tie a charger to a
UDC.

I'm thinking a phandle would be enough?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v7 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY

[Frank Wang]

Signed-off-by: Frank Wang 
Acked-by: Rob Herring 
Reviewed-by: Heiko Stuebner 
---

Changes in v7: None

Changes in v6:
 - Changed '_' to '-' for otg-id and otg-bvalid property.

Changes in v5:
 - Added 'reg' property to identify the different phy-blocks.

Changes in v4:
 - Used 'phy-supply' instead of 'vbus_*-supply'.

Changes in v3:
 - Added 'clocks' and 'clock-names' optional properties.
 - Specified 'otg-port' and 'host-port' as the sub-node name.

Changes in v2:
 - Changed vbus_host optional property from gpio to regulator.
 - Specified vbus_otg-supply optional property.
 - Specified otg_id and otg_bvalid property.

 .../bindings/phy/phy-rockchip-inno-usb2.txt|   64 
 1 file changed, 64 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt

diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt 
b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
new file mode 100644
index 000..3c29c77
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
@@ -0,0 +1,64 @@
+ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
+
+Required properties (phy (parent) node):
+ - compatible : should be one of the listed compatibles:
+   * "rockchip,rk3366-usb2phy"
+   * "rockchip,rk3399-usb2phy"
+ - reg : the address offset of grf for usb-phy configuration.
+ - #clock-cells : should be 0.
+ - clock-output-names : specify the 480m output clock name.
+
+Optional properties:
+ - clocks : phandle + phy specifier pair, for the input clock of phy.
+ - clock-names : input clock name of phy, must be "phyclk".
+
+Required nodes : a sub-node is required for each port the phy provides.
+The sub-node name is used to identify host or otg port,
+and shall be the following entries:
+   * "otg-port" : the name of otg port.
+   * "host-port" : the name of host port.
+
+Required properties (port (child) node):
+ - #phy-cells : must be 0. See ./phy-bindings.txt for details.
+ - interrupts : specify an interrupt for each entry in interrupt-names.
+ - interrupt-names : a list which shall be the following entries:
+   * "otg-id" : for the otg id interrupt.
+   * "otg-bvalid" : for the otg vbus interrupt.
+   * "linestate" : for the host/otg linestate interrupt.
+
+Optional properties:
+ - phy-supply : phandle to a regulator that provides power to VBUS.
+   See ./phy-bindings.txt for details.
+
+Example:
+
+grf: syscon@ff77 {
+   compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
+   #address-cells = <1>;
+   #size-cells = <1>;
+
+...
+
+   u2phy: usb2-phy@700 {
+   compatible = "rockchip,rk3366-usb2phy";
+   reg = <0x700 0x2c>;
+   #clock-cells = <0>;
+   clock-output-names = "sclk_otgphy0_480m";
+
+   u2phy_otg: otg-port {
+   #phy-cells = <0>;
+   interrupts = ,
+,
+;
+   interrupt-names = "otg-id", "otg-bvalid", "linestate";
+   status = "okay";
+   };
+
+   u2phy_host: host-port {
+   #phy-cells = <0>;
+   interrupts = ;
+   interrupt-names = "linestate";
+   status = "okay";
+   };
+   };
+};
-- 
1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
>
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.
>
> The usb charger will register notifiees on the usb gadget or the extcon
> device to get notified the usb charger state. It also supplies the
> notification mechanism to userspace When the usb charger state is changed.
>
> Power user will register a notifiee on the usb charger to get notified
> by status changes from the usb charger. It will report to power user
> to set the current limitation when detecting the usb charger is added
> or removed from extcon device state or usb gadget state.
>
> This patch doesn't yet integrate with the gadget code, so some functions
> which rely on the 'gadget' are not completed, that will be implemented
> in the following patches.
>
> Signed-off-by: Baolin Wang 
> Reviewed-by: Li Jun 
> Tested-by: Li Jun 
> ---
>  drivers/usb/gadget/Kconfig   |7 +
>  drivers/usb/gadget/udc/Makefile  |1 +
>  drivers/usb/gadget/udc/charger.c |  770 
> ++
>  include/linux/usb/charger.h  |  191 ++
>  include/uapi/linux/usb/charger.h |   31 ++
>  5 files changed, 1000 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 2057add..89f4e9b 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>   help
>  It supports the serial gadget can be used as a console.
>  
> +config USB_CHARGER
> + bool "USB charger support"

you didn't build test all possibilities, did you?

I have a feeling this won't link if USB_GADGET=m. Can you test that?

> diff --git a/drivers/usb/gadget/udc/charger.c 
> b/drivers/usb/gadget/udc/charger.c
[...]
> +struct class *usb_charger_class;

We already have a UDC class, do we really, really need another class
here?

> +subsys_initcall(usb_charger_class_init);

this should always work as module_init(). Please make sure that's the
case.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 13/14] usb: dwc3: host: use build-in property instead of platform data

[Mathias Nyman]

From: Heikki Krogerus 

This should allow xhci to remove handling of platform data.

Signed-off-by: Heikki Krogerus 
Cc: Felipe Balbi 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/dwc3/host.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..67f90d7 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -16,14 +16,13 @@
  */
 
 #include 
-#include 
 
 #include "core.h"
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
+   struct property_entry   props[2];
struct platform_device  *xhci;
-   struct usb_xhci_pdata   pdata;
int ret;
 
xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
@@ -47,14 +46,15 @@ int dwc3_host_init(struct dwc3 *dwc)
goto err1;
}
 
-   memset(, 0, sizeof(pdata));
+   memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
-   pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
-
-   ret = platform_device_add_data(xhci, , sizeof(pdata));
-   if (ret) {
-   dev_err(dwc->dev, "couldn't add platform data to xHCI 
device\n");
-   goto err1;
+   if (dwc->usb3_lpm_capable) {
+   props[0].name = "usb3-lpm-capable";
+   ret = platform_device_add_properties(xhci, props);
+   if (ret) {
+   dev_err(dwc->dev, "failed to add properties to xHCI\n");
+   goto err1;
+   }
}
 
phy_create_lookup(dwc->usb2_generic_phy, "usb2-phy",
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/8] usb: phy: msm: various cleanups

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> I stumbled over this warning last week, which showed up after I had
> removed an incorrect patch from my randconfig build setup:
>
> drivers/usb/phy/phy-msm-usb.c: In function 'msm_otg_probe':
> drivers/usb/phy/phy-msm-usb.c:1735:14: error: 'regs[0].consumer' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>   motg->vddcx = regs[0].consumer;
>   ^~
> drivers/usb/phy/phy-msm-usb.c:1736:14: error: 'regs[1].consumer' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>   motg->v3p3  = regs[1].consumer;
>   ^~
> drivers/usb/phy/phy-msm-usb.c:1737:14: error: 'regs[2].consumer' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>   motg->v1p8  = regs[2].consumer;
>   ^~
>
> Having already fixed the same problem in the phy-qcom-8x16-usb.c
> driver before, I tried to do the same thing here, but it turned
> out to be somewhat different, and I ended up running into several
> unrelated issues in the driver that I now try to fix up.
>
> The series is not tested beyond verifying that it no longer causes
> randconfig warnings, and some patches are not entirely obvious.
> In particular the ehci-msm and chipidea changes are probably a
> good idea, but they actually change the behavior of the drivers
> in a way that I cannot verify through inspection alone. The
> last patch in the series probably requires some changes to the
> devicetree files to go along with it.
>
> Please have a look and test this, provided the patches make sense
> conceptually.
>
>   Arnd
>
> Arnd Bergmann (8):
>   usb: phy: move msm_hsusb.h into driver
>   usb: ehci-msm: call usb_phy_init instead of open-coding it
>   usb: chipidea: msm: remove open-coded phy init
>   usb: phy: move TCSR driver into new file
>   usb: phy: msm: move register definitions into driver
>   usb: phy: qcom: use bulk regulator interfaces
>   usb: phy: msm: remove v1p8/v3p3 voltage setting
>   usb: phy: msm: disable regulator for remove()

I managed to apply a few of these. Some didn't apply. If you can rebase
on my testing/next and fix comments, then I can apply the rest.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >> So far, I haven't seen anybody talking about real USB OTG (the spec)
>> >> when they say OTG. Usually they just mean "a method for swapping between
>> >> host and peripheral roles, but we really don't want all the extra cost
>> >> of the OTG specification".
>> >> 
>> >
>> > That's what I thought before, but the request from the Marketing guy is
>> > "To prove the SoC is OTG compliance, support HNP and SRP", don't you
>> > see the SoC reference manual say "it supports HNP and SRP"?
>> >
>> > If there is no request, who else wants to implement so complicated FSM
>> > but seldom use cases, and go to pass OTG compliance test (tested by PET).
>> 
>> I stand corrected :-)
>> 
>> So there is one user for this layer. And this user has its own role
>> control registers. I'm not convinced we need this large generic layer
>> for one user.
>> 
>
> You mean chipidea or dwc3? I have more comments below.

chipidea. From the point of OTG (or DRD) dwc3 is very
self-sufficient. HW itself tracks state machine, much like MUSB does.

>> >> > For the real use case, some Carplay platforms need it.
>> >> 
>> >> Carplay does *NOT* rely on OTG. Apple has its own proprietary and closed
>> >> specification which is not OTG-compliant.
>> >> 
>> >
>> > Yes, it is not OTG-compliant, but it can co-work with some standard OTG FSM
>> > states to finish role swap.
>> 
>> What are you referring to as "finish role swap"? I don't get that.
>
> Change current role from host to peripheral.

Okay, we have two scenarios here:

1. You need full OTG compliance

For this, granted, you need the state machine if your HW doesn't
track it. This is a given. With only one user, however, perhaps
we don't need a generic layer. There are not enough different
setups to design a good enough generic layer. We will end up
with a pseudo-generic framework which is coupled with its only
user.

2. Dual-role support, without OTG compliance

In this case, you don't need a stack. All you need is a signal
to tell you state of ID pin and another to tell you state of
VBUS level. If you have those, you don't need to walk an OTG
state machine at all. You don't need any of those quirky OTG
timers, agreed?

Given the above, why would you even want to use a subset of OTG
state machine to implement something that's _usually_ as simple
as:

8<--
vbus = read(VBUS_STATE); /* could be a gpio_get_value() */
id = read(ID_STATE); /* could be a gpio_get_value() */

set_role(id);
set_vbus(vbus);


>> > Notice, it needs to swap role without disconnect cable.
>> 
>> right, I can swap role without changing cable, but that's not OTG. The
>> mechanism for that, AFAICT, is not HNP. I don't know details about
>> CarPlay because the spec isn't public, but my understanding is that
>> CarPlay doesn't rely on anything from OTG spec.
>
> Since it is non-public, I can't say much. Some flows of its role-swap
> refers to On-The-Go and Embedded Host Supplement to the USB Revision 2.0
> Specification. 
>
> But OTG FSM is not the only way, the platform which can do role-swap
> without disconnection can support it too.

Right, all you need for CarPlay is what I wrote above. You don't need
full OTG compliance for that, right?

>> >> >> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> >> >> > index f4fc0aa..1d74fb8 100644
>> >> >> > --- a/include/linux/usb/gadget.h
>> >> >> > +++ b/include/linux/usb/gadget.h
>> >> >> > @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>> >> >> >   * @in_epnum: last used in ep number
>> >> >> >   * @mA: last set mA value
>> >> >> >   * @otg_caps: OTG capabilities of this gadget.
>> >> >> > + * @otg_dev: OTG controller device, if needs to be used with OTG 
>> >> >> > core.
>> >> >> 
>> >> >> do you really know of any platform which has a separate OTG controller?
>> >> >> 
>> >> >
>> >> > It may not be a real separate OTG controller. It can be a hardware part
>> >> > (external connector, external IC, SoC OTG register area, etc) to handle 
>> >> > vbus
>> >> > ,id and other signals which are used for role swap.
>> >> 
>> >> That's already solved. EXTCON solved that years back and OMAP has been
>> >> using EXTCON to program its UTMI mailbox.
>> >> 
>> >
>> > No, that's not the same thing, it does not include the swap role.
>> 
>> Read your original comment:
>> 
>> "handle vbus, id and other signals which are *used for* role swap"
>> 
>> You didn't include role swap in your original comment. Semantics aside...
>> 
>> > Consider the use case the host driver is at host/ and udc driver is
>> > at gadget/udc, how to finish to role swap?
>> 
>> ... why does the source code placement matter? And what do you mean by
>> "finish role 

Re: [PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Heiko Stübner]

Hi Frank,

Am Dienstag, 21. Juni 2016, 15:52:45 schrieb Frank Wang:
> On 2016/6/20 12:56, Guenter Roeck wrote:
> > On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang  
wrote:
> >>> Turns out my problem was one of terminology. Using "suspend" and
> >>> "resume" to me suggested the common use of suspend and resume
> >>> functions. That is not the case here. After mentally replacing
> >>> "suspend" with "power_off" and "resume" with "power_on", you are
> >>> right, no problem exists. Sorry for the noise.
> >>> 
> >>> Maybe it would be useful to replace "resume" with "power_on" and
> >>> "suspend" with "power_off" in the function and variable names to
> >>> reduce confusion and misunderstandings.
> >>> 
> >>> Thanks,
> >>> Guenter
> >> 
> >> Well, it does have a bits confusion, however, the phy-port always just
> >> goes
> >> to suspend and resume mode (Not power off and power on) in a fact. So
> >> must
> >> it be renamed?
> > 
> > Other phy drivers name the functions _power_off and _power_on and
> > avoid the confusion. The callbacks are named .power_off and .power_on,
> > which gives a clear indication of its intended purpose. Other drivers
> > implementing suspend/resume (such as the omap usb phy driver) tie
> > those functions not into the power_off/power_on callbacks, but into
> > the driver's suspend/resume callbacks. At least the omap driver has
> > separate power management functions.
> > 
> > Do the functions _have_ to be renamed ? Surely not. But, if the
> > functions are really suspend/resume functions and not
> > power_off/power_on functions, maybe they should tie to the
> > suspend/resume functions and not register themselves as
> > power_off/power_on functions ?
> 
> As Guenter mentioned above,  I doped out two solutions, one is that keep
> current process but renaming *_resume/*_suspend to
> *_power_on/*_power_off;

in a way, naming stuff "power_off", "power_on" actually matches. For one, the 
phy-block goes from unusable to usable by usb-devices and also will power-on a 
phy-supply regulator (often named vcc_host* on Rockchip boards) from the phy-
core.

> another is that do not assign power_on/power_off
> functions for phy_ops at phy creating time, then, shorten
> _SCHEDULE_DELAY_ delay time less that 10 Seconds, and the phy-port
> suspend/resume mechanism depend on _sm_work_ completely.

Which in turn would mean that we would always depend on a fully controllable 
phy block. Right now it seems, rk3036, rk3228, rk3368 (probably forgot some) 
have the same type of phy, but with at least the unplug-detection missing.
In its current form the driver can very well support those (later on) by 
simply working statically (only acting on phy_power callbacks and not going to 
suspend on its own).

Also having the work running in 10-second intervall seems wasteful.

> 
> So which is the better way from your view? or would you like to give
> other unique perceptions please?

As obvious from the above, I would prefer just renaming the functions :-)


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Frank Wang]

Hi Heiko,

On 2016/6/21 17:05, Heiko Stübner wrote:

Hi Frank,

Am Dienstag, 21. Juni 2016, 15:52:45 schrieb Frank Wang:

On 2016/6/20 12:56, Guenter Roeck wrote:

On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang 

wrote:

Turns out my problem was one of terminology. Using "suspend" and
"resume" to me suggested the common use of suspend and resume
functions. That is not the case here. After mentally replacing
"suspend" with "power_off" and "resume" with "power_on", you are
right, no problem exists. Sorry for the noise.

Maybe it would be useful to replace "resume" with "power_on" and
"suspend" with "power_off" in the function and variable names to
reduce confusion and misunderstandings.

Thanks,
Guenter

Well, it does have a bits confusion, however, the phy-port always just
goes
to suspend and resume mode (Not power off and power on) in a fact. So
must
it be renamed?

Other phy drivers name the functions _power_off and _power_on and
avoid the confusion. The callbacks are named .power_off and .power_on,
which gives a clear indication of its intended purpose. Other drivers
implementing suspend/resume (such as the omap usb phy driver) tie
those functions not into the power_off/power_on callbacks, but into
the driver's suspend/resume callbacks. At least the omap driver has
separate power management functions.

Do the functions _have_ to be renamed ? Surely not. But, if the
functions are really suspend/resume functions and not
power_off/power_on functions, maybe they should tie to the
suspend/resume functions and not register themselves as
power_off/power_on functions ?

As Guenter mentioned above,  I doped out two solutions, one is that keep
current process but renaming *_resume/*_suspend to
*_power_on/*_power_off;

in a way, naming stuff "power_off", "power_on" actually matches. For one, the
phy-block goes from unusable to usable by usb-devices and also will power-on a
phy-supply regulator (often named vcc_host* on Rockchip boards) from the phy-
core.


another is that do not assign power_on/power_off
functions for phy_ops at phy creating time, then, shorten
_SCHEDULE_DELAY_ delay time less that 10 Seconds, and the phy-port
suspend/resume mechanism depend on _sm_work_ completely.

Which in turn would mean that we would always depend on a fully controllable
phy block. Right now it seems, rk3036, rk3228, rk3368 (probably forgot some)
have the same type of phy, but with at least the unplug-detection missing.
In its current form the driver can very well support those (later on) by
simply working statically (only acting on phy_power callbacks and not going to
suspend on its own).

Also having the work running in 10-second intervall seems wasteful.


So which is the better way from your view? or would you like to give
other unique perceptions please?

As obvious from the above, I would prefer just renaming the functions :-)

Heiko



Got it, thanks for your comments. I am going to correct and send out a 
new version later today, so sorry for trouble you to sign 'Reviewed-by' 
again if no any other issues.



BR.
Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 10:26:00AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> >> 
> >> So far, I haven't seen anybody talking about real USB OTG (the spec)
> >> when they say OTG. Usually they just mean "a method for swapping between
> >> host and peripheral roles, but we really don't want all the extra cost
> >> of the OTG specification".
> >> 
> >
> > That's what I thought before, but the request from the Marketing guy is
> > "To prove the SoC is OTG compliance, support HNP and SRP", don't you
> > see the SoC reference manual say "it supports HNP and SRP"?
> >
> > If there is no request, who else wants to implement so complicated FSM
> > but seldom use cases, and go to pass OTG compliance test (tested by PET).
> 
> I stand corrected :-)
> 
> So there is one user for this layer. And this user has its own role
> control registers. I'm not convinced we need this large generic layer
> for one user.
> 

You mean chipidea or dwc3? I have more comments below.

> >> > For the real use case, some Carplay platforms need it.
> >> 
> >> Carplay does *NOT* rely on OTG. Apple has its own proprietary and closed
> >> specification which is not OTG-compliant.
> >> 
> >
> > Yes, it is not OTG-compliant, but it can co-work with some standard OTG FSM
> > states to finish role swap.
> 
> What are you referring to as "finish role swap"? I don't get that.

Change current role from host to peripheral.

> 
> > Notice, it needs to swap role without disconnect cable.
> 
> right, I can swap role without changing cable, but that's not OTG. The
> mechanism for that, AFAICT, is not HNP. I don't know details about
> CarPlay because the spec isn't public, but my understanding is that
> CarPlay doesn't rely on anything from OTG spec.

Since it is non-public, I can't say much. Some flows of its role-swap
refers to On-The-Go and Embedded Host Supplement to the USB Revision 2.0
Specification. 

But OTG FSM is not the only way, the platform which can do role-swap
without disconnection can support it too.

> 
> >> >> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> >> > index f4fc0aa..1d74fb8 100644
> >> >> > --- a/include/linux/usb/gadget.h
> >> >> > +++ b/include/linux/usb/gadget.h
> >> >> > @@ -328,6 +328,7 @@ struct usb_gadget_ops {
> >> >> >   * @in_epnum: last used in ep number
> >> >> >   * @mA: last set mA value
> >> >> >   * @otg_caps: OTG capabilities of this gadget.
> >> >> > + * @otg_dev: OTG controller device, if needs to be used with OTG 
> >> >> > core.
> >> >> 
> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> 
> >> >
> >> > It may not be a real separate OTG controller. It can be a hardware part
> >> > (external connector, external IC, SoC OTG register area, etc) to handle 
> >> > vbus
> >> > ,id and other signals which are used for role swap.
> >> 
> >> That's already solved. EXTCON solved that years back and OMAP has been
> >> using EXTCON to program its UTMI mailbox.
> >> 
> >
> > No, that's not the same thing, it does not include the swap role.
> 
> Read your original comment:
> 
> "handle vbus, id and other signals which are *used for* role swap"
> 
> You didn't include role swap in your original comment. Semantics aside...
> 
> > Consider the use case the host driver is at host/ and udc driver is
> > at gadget/udc, how to finish to role swap?
> 
> ... why does the source code placement matter? And what do you mean by
> "finish role swap"?
> 

Well, it depends on your driver design, do you want the host driver's
API is still be called when current role is peripheral? One typical
problem you can refer below:

commit 11c011a5e777c83819078a18672543f04482b3ec
Author: Srinivas Kandagatla 
Date:   Thu May 19 11:12:56 2016 +0100

usb: echi-hcd: Add ehci_setup check before echi_shutdown



In some cases, the USB code (gadget/hcd->start/stop) needs to be called
during the role swap. For example, if you have mux driver, you may
need to call usb_remove_hcd when ID from 0 to 1. Without Roger's framework,
how can we do that?

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/20/2016 10:44 PM, Heiko Stübner wrote:

Hi William,

Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:

On 06/17/2016 07:15 AM, Heiko Stübner wrote:

Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:

This patch adds the devicetree documentation required for Rockchip
USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.

It supports DRD mode, and could operate in device mode (SS, HS, FS)
and host mode (SS, HS, FS, LS).

Signed-off-by: William Wu 

[...]


+Optional clocks:
+  "aclk_usb3otg0"Aclk for specific usb controller clock.

what does this clock do? Also most likely same argument as below.

Here is partial rk3399 clk tree about usb3:

aclk_usb3   77   29700 0 0
 aclk_usb3_grf11 
29700  0 0
 aclk_usb3_rksoc_axi_perf 11 
29700  0 0
 aclk_usb3otg111 
29700  0 0
 aclk_usb3otg011 
29700  0 0
 aclk_usb3_noc11 
29700  0 0


from the clk tree, and check with our IC designers, we can see that:
1. aclk_usb3 is the parent clk of aclk_usb3
2. aclk_usb3_grf  is used for both otg0 and otg1 grf, and these usb3 grf 
can be set

to control otg0 and otg1 controller, but not the phy.
3. aclk_usb3otg1 is otg1 controller clk,  aclk_usb3otg0 is otg0 
controller clk.

4. aclk_usb3_rksoc_axi_perf is the clk for usb3 performance monitor module.
5. aclk_usb3_noc is the clk for soc bus interconnect.




+  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
platforms.

The clock names looks pretty strange. What are they for? Especially as
nothing seems to use them right now.

"aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the usb3
performance monitor control registers right now.

ok, then I'd suggest not defining the clock for now.

For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22, gmac, gpu,
etc) which all seem to share a somewhat similar design, so they will maybe
result in a separate driver of some form for the performance monitors.

And secondly, it is somewhat easy to add new optional properties, but you
cannot remove anything defined previously. So if we later decide to handle all
the performance monitors differently, you can't remove that clock from the
binding again. (or at least only with quite a bit of hassle).

So as this clock isn't used at all yet, I guess it should not get included
now.

Yes, I agree with you. We can remove the aclk_usb3_rksoc_axi_perf right now.



+  "aclk_usb3_grf"USB grf clock.  Not present on all platforms.

for my own education, which part of the GRF does this clock supply?

"aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX

Hmm, this looks more like it belongs to the otg phy?
Anyway, also seems unused right now, so same argument as above applies here.
As I have described above, the "aclk_usb3_grf" is  used for both otg0 
and otg1 grf,
and these usb3 grf can be set to control otg0 and otg1 controller, but 
not the phy.
And we have done a test that remove the grf clk, and the result showed 
that usb3

controller can't work normally. So I think we need the usb3 grf clk.

So about the usb3 controller clk management, I think it should contain 
the following clk:

1.  aclk_usb3otg1
2.  aclk_usb3otg0
3.  aclk_usb3_grf
4.  aclk_usb3_noc
For "aclk_usb3_noc", I discuss with our clk manager, the clk is always 
on before,
but according to upstream maintainer's suggestion, we need to manage the 
noc clk by each module.


and the follow two clk can be removed:
1. aclk_usb3
2. aclk_usb3_rksoc_axi_perf

Is it ok?



Heiko






--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v12 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger,
which is pending testing. Moreover there may be other potential users will use
it in future.

Changes since v11:
 - Reviewed and tested by Li Jun.

Changes since v10:
 - Introduce usb_charger_get_state() function to check charger state.
 - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
 in case will be issued in atomic context.

Baolin Wang (4):
  gadget: Introduce the usb charger framework
  gadget: Support for the usb charger framework
  gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c  |   69 
 drivers/usb/gadget/Kconfig|7 +
 drivers/usb/gadget/udc/Makefile   |1 +
 drivers/usb/gadget/udc/charger.c  |  807 +
 drivers/usb/gadget/udc/udc-core.c |   11 +
 include/linux/mfd/wm831x/pdata.h  |3 +
 include/linux/usb/charger.h   |  191 +
 include/linux/usb/gadget.h|   11 +
 include/uapi/linux/usb/charger.h  |   31 ++
 9 files changed, 1131 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

For supporting the usb charger, it adds the usb_charger_init() and
usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed,
then the usb charger can do the power things.

Signed-off-by: Baolin Wang 
Reviewed-by: Li Jun 
Tested-by: Li Jun 
---
 drivers/usb/gadget/udc/udc-core.c |   11 +++
 include/linux/usb/gadget.h|   11 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c 
b/drivers/usb/gadget/udc/udc-core.c
index 6e8300d..84c098c 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -242,6 +243,9 @@ static void usb_gadget_state_work(struct work_struct *work)
struct usb_gadget *gadget = work_to_gadget(work);
struct usb_udc *udc = gadget->udc;
 
+   /* when the gadget state is changed, then report to USB charger */
+   usb_charger_plug_by_gadget(gadget, gadget->state);
+
if (udc)
sysfs_notify(>dev.kobj, NULL, "state");
 }
@@ -411,6 +415,10 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
if (ret)
goto err4;
 
+   ret = usb_charger_init(gadget);
+   if (ret)
+   goto err5;
+
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
udc->vbus = true;
 
@@ -431,6 +439,8 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
return 0;
 
+err5:
+   device_del(>dev);
 err4:
list_del(>list);
mutex_unlock(_lock);
@@ -539,6 +549,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
kobject_uevent(>dev.kobj, KOBJ_REMOVE);
flush_work(>work);
device_unregister(>dev);
+   usb_charger_exit(gadget);
device_unregister(>dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 457651b..40390ec 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct usb_ep;
 
@@ -639,6 +640,8 @@ struct usb_gadget {
unsignedout_epnum;
unsignedin_epnum;
struct usb_otg_caps *otg_caps;
+   /* negotiate the power with the usb charger */
+   struct usb_charger  *charger;
 
unsignedsg_supported:1;
unsignedis_otg:1;
@@ -855,10 +858,18 @@ static inline int usb_gadget_vbus_connect(struct 
usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+   if (gadget->charger)
+   usb_charger_set_cur_limit_by_type(gadget->charger,
+ gadget->charger->type,
+ mA);
+
if (!gadget->ops->vbus_draw)
return -EOPNOTSUPP;
return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v12 3/4] gadget: Integrate with the usb gadget supporting for usb charger

[Baolin Wang]

When the usb gadget supporting for usb charger is ready, the usb charger
can implement the usb_charger_plug_by_gadget() function and usb_charger_exit()
function by getting 'struct usb_charger' from 'struct gadget'.

Signed-off-by: Baolin Wang 
Reviewed-by: Li Jun 
Tested-by: Li Jun 
---
 drivers/usb/gadget/udc/charger.c |   39 +-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
index 7be4c76..949396f 100644
--- a/drivers/usb/gadget/udc/charger.c
+++ b/drivers/usb/gadget/udc/charger.c
@@ -568,6 +568,30 @@ usb_charger_plug_by_extcon(struct notifier_block *nb,
 int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
   unsigned long state)
 {
+   struct usb_charger *uchger = gadget->charger;
+   enum usb_charger_state uchger_state;
+
+   if (WARN(!uchger, "charger can not be NULL"))
+   return -EINVAL;
+
+   /*
+* Report event to power to setting the current limitation
+* for this usb charger when one usb charger state is changed
+* with detecting by usb gadget state.
+*/
+   if (uchger->old_gadget_state != state) {
+   uchger->old_gadget_state = state;
+
+   if (state >= USB_STATE_ATTACHED)
+   uchger_state = USB_CHARGER_PRESENT;
+   else if (state == USB_STATE_NOTATTACHED)
+   uchger_state = USB_CHARGER_REMOVE;
+   else
+   uchger_state = USB_CHARGER_DEFAULT;
+
+   usb_charger_notify_others(uchger, uchger_state);
+   }
+
return 0;
 }
 EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
@@ -724,6 +748,7 @@ int usb_charger_init(struct usb_gadget *ugadget)
 
/* register a notifier on a usb gadget device */
uchger->gadget = ugadget;
+   ugadget->charger = uchger;
uchger->old_gadget_state = USB_STATE_NOTATTACHED;
 
/* register a new usb charger */
@@ -744,7 +769,19 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-   return 0;
+   struct usb_charger *uchger = ugadget->charger;
+
+   if (WARN(!uchger, "charger can not be NULL"))
+   return -EINVAL;
+
+   if (uchger->extcon_dev)
+   extcon_unregister_notifier(uchger->extcon_dev,
+  EXTCON_USB,
+  >extcon_nb.nb);
+
+   ida_simple_remove(_charger_ida, uchger->id);
+
+   return usb_charger_unregister(uchger);
 }
 
 static int __init usb_charger_class_init(void)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Integrate with the newly added USB charger interface to limit the current
we draw from the USB input based on the input device configuration
identified by the USB stack, allowing us to charge more quickly from high
current inputs without drawing more current than specified from others.

Signed-off-by: Mark Brown 
Signed-off-by: Baolin Wang 
Acked-by: Lee Jones 
Acked-by: Charles Keepax 
Acked-by: Peter Chen 
Acked-by: Sebastian Reichel 
---
 drivers/power/wm831x_power.c |   69 ++
 include/linux/mfd/wm831x/pdata.h |3 ++
 2 files changed, 72 insertions(+)

diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
index 7082301..cef1812 100644
--- a/drivers/power/wm831x_power.c
+++ b/drivers/power/wm831x_power.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -31,6 +32,8 @@ struct wm831x_power {
char usb_name[20];
char battery_name[20];
bool have_battery;
+   struct usb_charger *usb_charger;
+   struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In milliamps */
+static const unsigned int wm831x_usb_limits[] = {
+   0,
+   2,
+   100,
+   500,
+   900,
+   1500,
+   1800,
+   550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+  unsigned long limit, void *data)
+{
+   struct wm831x_power *wm831x_power = container_of(nb,
+struct wm831x_power,
+usb_notify);
+   unsigned int i, best;
+
+   /* Find the highest supported limit */
+   best = 0;
+   for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+   if (limit >= wm831x_usb_limits[i] &&
+   wm831x_usb_limits[best] < wm831x_usb_limits[i])
+   best = i;
+   }
+
+   dev_dbg(wm831x_power->wm831x->dev,
+   "Limiting USB current to %umA", wm831x_usb_limits[best]);
+
+   wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+   WM831X_USB_ILIM_MASK, best);
+
+   return 0;
+}
+
 /*
  * Battery properties
  */
@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
}
}
 
+   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+   power->usb_charger =
+   usb_charger_find_by_name(wm831x_pdata->usb_gadget);
+   if (IS_ERR(power->usb_charger)) {
+   ret = PTR_ERR(power->usb_charger);
+   dev_err(>dev,
+   "Failed to find USB gadget: %d\n", ret);
+   goto err_bat_irq;
+   }
+
+   power->usb_notify.notifier_call = wm831x_usb_limit_change;
+
+   ret = usb_charger_register_notify(power->usb_charger,
+ >usb_notify);
+   if (ret != 0) {
+   dev_err(>dev,
+   "Failed to register notifier: %d\n", ret);
+   goto err_usb_charger;
+   }
+   }
+
return ret;
 
+err_usb_charger:
+   /* put_device on charger */
 err_bat_irq:
--i;
for (; i >= 0; i--) {
@@ -637,6 +700,12 @@ static int wm831x_power_remove(struct platform_device 
*pdev)
struct wm831x *wm831x = wm831x_power->wm831x;
int irq, i;
 
+   if (wm831x_power->usb_charger) {
+   usb_charger_unregister_notify(wm831x_power->usb_charger,
+ _power->usb_notify);
+   /* Free charger */
+   }
+
for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
irq = wm831x_irq(wm831x, 
 platform_get_irq_byname(pdev,
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index dcc9631..5af8399 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -126,6 +126,9 @@ struct wm831x_pdata {
/** The driver should initiate a power off sequence during shutdown */
bool soft_shutdown;
 
+   /** dev_name of USB charger gadget to integrate with */
+   const char *usb_gadget;
+
int irq_base;
int gpio_base;
int gpio_defaults[WM831X_GPIO_NUM];
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe 

[PATCH v12 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

This patch introduces the usb charger driver based on usb gadget that
makes an enhancement to a power driver. It works well in practice but
that requires a system with suitable hardware.

The basic conception of the usb charger is that, when one usb charger
is added or removed by reporting from the usb gadget state change or
the extcon device state change, the usb charger will report to power
user to set the current limitation.

The usb charger will register notifiees on the usb gadget or the extcon
device to get notified the usb charger state. It also supplies the
notification mechanism to userspace When the usb charger state is changed.

Power user will register a notifiee on the usb charger to get notified
by status changes from the usb charger. It will report to power user
to set the current limitation when detecting the usb charger is added
or removed from extcon device state or usb gadget state.

This patch doesn't yet integrate with the gadget code, so some functions
which rely on the 'gadget' are not completed, that will be implemented
in the following patches.

Signed-off-by: Baolin Wang 
Reviewed-by: Li Jun 
Tested-by: Li Jun 
---
 drivers/usb/gadget/Kconfig   |7 +
 drivers/usb/gadget/udc/Makefile  |1 +
 drivers/usb/gadget/udc/charger.c |  770 ++
 include/linux/usb/charger.h  |  191 ++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 2057add..89f4e9b 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
help
   It supports the serial gadget can be used as a console.
 
+config USB_CHARGER
+   bool "USB charger support"
+   help
+ The usb charger driver based on the usb gadget that makes an
+ enhancement to a power driver which can set the current limitation
+ when the usb charger is added or removed.
+
 source "drivers/usb/gadget/udc/Kconfig"
 
 #
diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile
index dfee534..0e9564c 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -2,6 +2,7 @@
 # USB peripheral controller drivers
 #
 obj-$(CONFIG_USB_GADGET)   += udc-core.o
+obj-$(CONFIG_USB_CHARGER)  += charger.o
 obj-$(CONFIG_USB_DUMMY_HCD)+= dummy_hcd.o
 obj-$(CONFIG_USB_NET2272)  += net2272.o
 obj-$(CONFIG_USB_NET2280)  += net2280.o
diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
new file mode 100644
index 000..7be4c76
--- /dev/null
+++ b/drivers/usb/gadget/udc/charger.c
@@ -0,0 +1,770 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_SDP_CUR_LIMIT  500
+#define DEFAULT_SDP_CUR_LIMIT_SS   900
+#define DEFAULT_DCP_CUR_LIMIT  1500
+#define DEFAULT_CDP_CUR_LIMIT  1500
+#define DEFAULT_ACA_CUR_LIMIT  1500
+
+static DEFINE_IDA(usb_charger_ida);
+struct class *usb_charger_class;
+static unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+static struct usb_charger *dev_to_uchger(struct device *dev)
+{
+   return container_of(dev, struct usb_charger, dev);
+}
+
+/*
+ * charger_current_show() - Show the charger current limit.
+ */
+static ssize_t charger_current_show(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   struct usb_charger *uchger = dev_to_uchger(dev);
+
+   return sprintf(buf, "%u\n", usb_charger_get_cur_limit(uchger));
+}
+static DEVICE_ATTR_RO(charger_current);
+
+/*
+ * charger_type_show() - Show the charger type.
+ *
+ * It can be SDP/DCP/CDP/ACA type, else for unknown type.
+ */
+static ssize_t charger_type_show(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct usb_charger *uchger = dev_to_uchger(dev);
+   int cnt;
+
+   switch (uchger->type) {
+   case SDP_TYPE:
+   cnt = sprintf(buf, "%s\n", "SDP");
+   break;
+   case DCP_TYPE:
+   cnt = sprintf(buf, "%s\n", "DCP");
+   break;
+   case CDP_TYPE:
+   cnt = sprintf(buf, "%s\n", "CDP");
+   break;
+   case ACA_TYPE:
+   

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> Peter Chen  writes:
>> >> >>> +
>> >> >>> + /* start host */
>> >> >>> + ret = hcd_ops->add(otg->primary_hcd.hcd,
>> >> >>> +otg->primary_hcd.irqnum,
>> >> >>> +otg->primary_hcd.irqflags);
>> >> >> 
>> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
>> >> >
>> >> > I've introduced the host and gadget ops interface to get around the
>> >> > circular dependency issue we can't avoid.
>> >> > otg needs to call host/gadget functions and host/gadget also needs to
>> >> > call otg functions.
>> >> 
>> >> IMO, this shows a fragility of your design. You're, now, lying to
>> >> usb_hcd and usb_udc and making them register into a virtual layer that
>> >> doesn't exist. And that layer will end up calling the real registration
>> >> function when some magic event happens.
>> >> 
>> >> This is only really needed for quirky devices like dwc3 (but see more on
>> >> dwc3 below) where host and peripheral registers shadow each
>> >> other. Otherwise we would be able to always keep hcd and udc always
>> >> registered. They would get different interrupt statuses anyway and
>> >> nothing would ever break.
>> >> 
>> >> However, when it comes to dwc3, we already have all the code necessary
>> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
>> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
>> >> the OTG states for those folks who really care about OTG (Hint: nobody
>> >> has cared for the past 10 years, why would they do so now?) and we don't
>> >> need a SW state machine when the HW handles that for us, right?
>> >> 
>> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
>> >> I know very little about that IP and don't even have documentation on
>> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
>> >> which changes roles automatically in HW based on ID pin state.
>> >
>> > Chipidea needs to set register for USB role manually.
>> 
>> okay, so chipidea has private control of role. Much like dwc3. That's good.
>> 
>> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG 
>> >> >>> core.
>> >> >> 
>> >> >> do you really know of any platform which has a separate OTG controller?
>> >> >> 
>> >> >
>> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for 
>> >> > OTG, host
>> >> > and gadget.
>> >> >
>> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> 
>> >> that's not an OTG controller, it's just a mux. No different than Intel's
>> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> 
>> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> >> are two competing standards and, apparently, type-C is winning when it
>> >> comes to role-swapping.
>> >> 
>> >
>> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
>> > It will be considered as an OTG port.
>> 
>> That's because "dual-role" is a non-standard OTG. Seen as people really
>> didn't care about OTG, we (linux-usb folks) ended up naturally referring
>> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>
> So, unless we use OTG FSM defined in OTG spec, we should not mention
> "OTG" in Linux, right?

to avoid confusion with the terminology, yes. With that settled, let's
figure out how you can deliver what your marketting guys are asking of
you.

>> > You are right, if the connector is type-c, it will be called as "type-c
>> > port" by people :)
>> 
>> oh no, that's not what I'm talking about. If you read Type-C and PD
>> specs, they define their own method for data role swapping. USB OTG
>> doesn't fit on top of a Type-C environment. It's not about what people
>> will call it, it's really that OTG can't work on top of type-c. For
>> starters, there's no ID pin ;-)
>
> I know type-c, yes, there is no relationship between OTG and type-c.

okay, thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Felipe Balbi]

Hi,

William Wu  writes:
> This patch adds the devicetree documentation required for Rockchip
> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>
> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> and host mode (SS, HS, FS, LS).
>
> Signed-off-by: William Wu 

I'm dropping this series until comments are sorted out.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, Jun 21, 2016 at 10:19:32AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> >> >>> +
> >> >>> +  /* start host */
> >> >>> +  ret = hcd_ops->add(otg->primary_hcd.hcd,
> >> >>> + otg->primary_hcd.irqnum,
> >> >>> + otg->primary_hcd.irqflags);
> >> >> 
> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
> >> >
> >> > I've introduced the host and gadget ops interface to get around the
> >> > circular dependency issue we can't avoid.
> >> > otg needs to call host/gadget functions and host/gadget also needs to
> >> > call otg functions.
> >> 
> >> IMO, this shows a fragility of your design. You're, now, lying to
> >> usb_hcd and usb_udc and making them register into a virtual layer that
> >> doesn't exist. And that layer will end up calling the real registration
> >> function when some magic event happens.
> >> 
> >> This is only really needed for quirky devices like dwc3 (but see more on
> >> dwc3 below) where host and peripheral registers shadow each
> >> other. Otherwise we would be able to always keep hcd and udc always
> >> registered. They would get different interrupt statuses anyway and
> >> nothing would ever break.
> >> 
> >> However, when it comes to dwc3, we already have all the code necessary
> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> >> the OTG states for those folks who really care about OTG (Hint: nobody
> >> has cared for the past 10 years, why would they do so now?) and we don't
> >> need a SW state machine when the HW handles that for us, right?
> >> 
> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> >> I know very little about that IP and don't even have documentation on
> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> >> which changes roles automatically in HW based on ID pin state.
> >
> > Chipidea needs to set register for USB role manually.
> 
> okay, so chipidea has private control of role. Much like dwc3. That's good.
> 
> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> >> 
> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> 
> >> >
> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, 
> >> > host
> >> > and gadget.
> >> >
> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> 
> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> 
> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> are two competing standards and, apparently, type-C is winning when it
> >> comes to role-swapping.
> >> 
> >
> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> > It will be considered as an OTG port.
> 
> That's because "dual-role" is a non-standard OTG. Seen as people really
> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> to "non-standard OTG" as "dual-role". Just to avoid confusion.

So, unless we use OTG FSM defined in OTG spec, we should not mention
"OTG" in Linux, right?
 
> 
> > You are right, if the connector is type-c, it will be called as "type-c
> > port" by people :)
> 
> oh no, that's not what I'm talking about. If you read Type-C and PD
> specs, they define their own method for data role swapping. USB OTG
> doesn't fit on top of a Type-C environment. It's not about what people
> will call it, it's really that OTG can't work on top of type-c. For
> starters, there's no ID pin ;-)

I know type-c, yes, there is no relationship between OTG and type-c.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH] usb: dwc2: Add reset control to dwc2

[Felipe Balbi]

Hi,

John Youn  writes:
> On 6/3/2016 8:59 AM, dingu...@opensource.altera.com wrote:
>> From: Dinh Nguyen 
>> 
>> Allow for platforms that have a reset controller driver in place to bring
>> the USB IP out of reset.
>> 
>> Signed-off-by: Dinh Nguyen 
>> Acked-by: John Youn 
>> Tested-by: Stefan Wahren 
>> ---
>> v7: Use devm_reset_control_get_optional()
>> v6: fix 80 line checkpatch warning in dev_err print
>> v5: updated error conditions for not finding the reset property
>> v4: use dev_dbg() if not a -EPROBE_DEFER
>> v3: fix compile error
>> v2: move to lowlevel_hw_init()
>> ---
>>  drivers/usb/dwc2/core.h |1 +
>>  drivers/usb/dwc2/platform.c |   22 ++
>>  2 files changed, 23 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 3c58d63..f748132 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -837,6 +837,7 @@ struct dwc2_hsotg {
>>  void *priv;
>>  int irq;
>>  struct clk *clk;
>> +struct reset_control *reset;
>>  
>>  unsigned int queuing_high_bandwidth:1;
>>  unsigned int srp_success:1;
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 88629be..d34f169 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -45,6 +45,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  
>> @@ -337,6 +338,24 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg 
>> *hsotg)
>>  {
>>  int i, ret;
>>  
>> +hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
>> +if (IS_ERR(hsotg->reset)) {
>> +ret = PTR_ERR(hsotg->reset);
>> +switch (ret) {
>> +case -ENOENT:
>> +case -ENOTSUPP:
>> +hsotg->reset = NULL;
>> +break;
>> +default:
>> +dev_err(hsotg->dev, "error getting reset control %d\n",
>> +ret);
>> +return ret;
>> +}
>> +}
>> +
>> +if (hsotg->reset)
>> +reset_control_deassert(hsotg->reset);
>> +
>>  /* Set default UTMI width */
>>  hsotg->phyif = GUSBCFG_PHYIF16;
>>  
>> @@ -434,6 +453,9 @@ static int dwc2_driver_remove(struct platform_device 
>> *dev)
>>  if (hsotg->ll_hw_enabled)
>>  dwc2_lowlevel_hw_disable(hsotg);
>>  
>> +if (hsotg->reset)
>> +reset_control_assert(hsotg->reset);
>> +
>>  return 0;
>>  }
>>  
>> 
>
> Hi Felipe,
>
> This patch depends on this one in the reset control tree.
>
> http://marc.info/?l=linux-usb=146473891018262=2
>
> Should you also have this patch in your tree? Or what is the best way
> to deal with it?

We have two ways of dealing with this:

1. We wait for v4.9
2. Reset folks take this patch together with the rest of the changes

I'm fine with either way. Should (2) be chosen here's my ack:

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 11/14] xhci: rename and simplify last_trb_on_last_seg() helper

[Mathias Nyman]

It's only used with rings that have link trbs

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 65 +---
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2d8dcb1f..26ed512 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -89,19 +89,6 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg,
return seg->dma + (segment_offset * sizeof(*trb));
 }
 
-/* Does this link TRB point to the first segment in a ring,
- * or was the previous TRB the last TRB on the last segment in the ERST?
- */
-static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
-   struct xhci_segment *seg, union xhci_trb *trb)
-{
-   if (ring == xhci->event_ring)
-   return (trb == >trbs[TRBS_PER_SEGMENT]) &&
-   (seg->next == xhci->event_ring->first_seg);
-   else
-   return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
-}
-
 static bool trb_is_link(union xhci_trb *trb)
 {
return TRB_TYPE_LINK_LE32(trb->link.control);
@@ -118,6 +105,11 @@ static bool last_trb_on_ring(struct xhci_ring *ring,
return last_trb_on_seg(seg, trb) && (seg->next == ring->first_seg);
 }
 
+static bool link_trb_toggles_cycle(union xhci_trb *trb)
+{
+   return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
+}
+
 /* Updates trb to point to the next TRB in the ring, and updates seg if the 
next
  * TRB is in a new segment.  This does not skip over link TRBs, and it does not
  * effect the ring dequeue or enqueue pointers.
@@ -226,7 +218,7 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring 
*ring,
next->link.control ^= cpu_to_le32(TRB_CYCLE);
 
/* Toggle the cycle bit after the last ring segment. */
-   if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next))
+   if (link_trb_toggles_cycle(next))
ring->cycle_state ^= 1;
 
ring->enq_seg = ring->enq_seg->next;
@@ -2867,36 +2859,29 @@ static int prepare_ring(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
}
}
 
-   if (trb_is_link(ep_ring->enqueue)) {
-   struct xhci_ring *ring = ep_ring;
-   union xhci_trb *next;
-
-   next = ring->enqueue;
+   while (trb_is_link(ep_ring->enqueue)) {
+   /* If we're not dealing with 0.95 hardware or isoc rings
+* on AMD 0.96 host, clear the chain bit.
+*/
+   if (!xhci_link_trb_quirk(xhci) &&
+   !(ep_ring->type == TYPE_ISOC &&
+ (xhci->quirks & XHCI_AMD_0x96_HOST)))
+   ep_ring->enqueue->link.control &=
+   cpu_to_le32(~TRB_CHAIN);
+   else
+   ep_ring->enqueue->link.control |=
+   cpu_to_le32(TRB_CHAIN);
 
-   while (trb_is_link(next)) {
-   /* If we're not dealing with 0.95 hardware or isoc rings
-* on AMD 0.96 host, clear the chain bit.
-*/
-   if (!xhci_link_trb_quirk(xhci) &&
-   !(ring->type == TYPE_ISOC &&
-(xhci->quirks & XHCI_AMD_0x96_HOST)))
-   next->link.control &= cpu_to_le32(~TRB_CHAIN);
-   else
-   next->link.control |= cpu_to_le32(TRB_CHAIN);
+   wmb();
+   ep_ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE);
 
-   wmb();
-   next->link.control ^= cpu_to_le32(TRB_CYCLE);
+   /* Toggle the cycle bit after the last ring segment. */
+   if (link_trb_toggles_cycle(ep_ring->enqueue))
+   ep_ring->cycle_state ^= 1;
 
-   /* Toggle the cycle bit after the last ring segment. */
-   if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, 
next)) {
-   ring->cycle_state ^= 1;
-   }
-   ring->enq_seg = ring->enq_seg->next;
-   ring->enqueue = ring->enq_seg->trbs;
-   next = ring->enqueue;
-   }
+   ep_ring->enq_seg = ep_ring->enq_seg->next;
+   ep_ring->enqueue = ep_ring->enq_seg->trbs;
}
-
return 0;
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v11 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

Hi Felipe,

On 21 June 2016 at 15:45, Felipe Balbi  wrote:
>
> Hi Baolin,
>
> Baolin Wang  writes:
 -Original Message-
 From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
 ow...@vger.kernel.org] On Behalf Of Baolin Wang
 Sent: Monday, June 13, 2016 4:47 PM
 To: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
 dbarysh...@gmail.com; dw...@infradead.org
 Cc: r...@kernel.org; m.szyprow...@samsung.com; ruslan.bilo...@gmail.com;
 peter.c...@freescale.com; st...@rowland.harvard.edu; r.bald...@samsung.com;
 yoshihiro.shimoda...@renesas.com; lee.jo...@linaro.org; broo...@kernel.org;
 ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
 baolin.w...@linaro.org; linux...@vger.kernel.org; linux-
 u...@vger.kernel.org; device-mainlin...@lists.linuxfoundation.org; linux-
 ker...@vger.kernel.org
 Subject: [RESEND PATCH v11 1/4] gadget: Introduce the usb charger
 framework

 This patch introduces the usb charger driver based on usb gadget that
 makes an enhancement to a power driver. It works well in practice but that
 requires a system with suitable hardware.

 The basic conception of the usb charger is that, when one usb charger is
 added or removed by reporting from the usb gadget state change or the
 extcon device state change, the usb charger will report to power user to
 set the current limitation.

 The usb charger will register notifiees on the usb gadget or the extcon
 device to get notified the usb charger state. It also supplies the
 notification mechanism to userspace When the usb charger state is changed.

 Power user will register a notifiee on the usb charger to get notified by
 status changes from the usb charger. It will report to power user to set
 the current limitation when detecting the usb charger is added or removed
 from extcon device state or usb gadget state.

 This patch doesn't yet integrate with the gadget code, so some functions
 which rely on the 'gadget' are not completed, that will be implemented in
 the following patches.

 Signed-off-by: Baolin Wang 
>>>
>>> Reviewed-by: Li Jun 
>>> Tested-by: Li Jun 
>>
>> Do you have any comments about this patchset, if not I will resend it
>> with testing by Li Jun (thanks for Jun reviewing and testing). Thanks.
>
> please resend. I don't have the series in my queue anymore, sorry.

OK. I would like to resend this patchset. Thanks.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/14] xhci: rework inc_deq() and fix off by one error.

[Mathias Nyman]

inc_deq() is called both for rings with link trbs and the event ring
without link trbs.
The last_trb() check in inc_deq() has a off by one error, going beyond
allocated array when checking if trb == [TRBS_PER_SEGMENT], and the whole
inc_deq() depend on this.

Rewrite the inc_deq() funciton, remove the faulty last_trb() helper, add
new last_trb_on_seg() and last_trb_on_ring() helpers

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 68 +---
 1 file changed, 32 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4de8a2b..086b871 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -102,19 +102,6 @@ static bool last_trb_on_last_seg(struct xhci_hcd *xhci, 
struct xhci_ring *ring,
return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
 }
 
-/* Is this TRB a link TRB or was the last TRB the last TRB in this event ring
- * segment?  I.e. would the updated event TRB pointer step off the end of the
- * event seg?
- */
-static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
-   struct xhci_segment *seg, union xhci_trb *trb)
-{
-   if (ring == xhci->event_ring)
-   return trb == >trbs[TRBS_PER_SEGMENT];
-   else
-   return TRB_TYPE_LINK_LE32(trb->link.control);
-}
-
 static bool trb_is_link(union xhci_trb *trb)
 {
return TRB_TYPE_LINK_LE32(trb->link.control);
@@ -126,6 +113,17 @@ static int enqueue_is_link_trb(struct xhci_ring *ring)
return TRB_TYPE_LINK_LE32(link->control);
 }
 
+static bool last_trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb)
+{
+   return trb == >trbs[TRBS_PER_SEGMENT - 1];
+}
+
+static bool last_trb_on_ring(struct xhci_ring *ring,
+   struct xhci_segment *seg, union xhci_trb *trb)
+{
+   return last_trb_on_seg(seg, trb) && (seg->next == ring->first_seg);
+}
+
 /* Updates trb to point to the next TRB in the ring, and updates seg if the 
next
  * TRB is in a new segment.  This does not skip over link TRBs, and it does not
  * effect the ring dequeue or enqueue pointers.
@@ -151,31 +149,29 @@ static void inc_deq(struct xhci_hcd *xhci, struct 
xhci_ring *ring)
 {
ring->deq_updates++;
 
-   /*
-* If this is not event ring, and the dequeue pointer
-* is not on a link TRB, there is one more usable TRB
-*/
-   if (ring->type != TYPE_EVENT && !trb_is_link(ring->dequeue))
-   ring->num_trbs_free++;
-
-   do {
-   /*
-* Update the dequeue pointer further if that was a link TRB or
-* we're at the end of an event ring segment (which doesn't have
-* link TRBS)
-*/
-   if (last_trb(xhci, ring, ring->deq_seg, ring->dequeue)) {
-   if (ring->type == TYPE_EVENT &&
-   last_trb_on_last_seg(xhci, ring,
-   ring->deq_seg, ring->dequeue)) {
-   ring->cycle_state ^= 1;
-   }
-   ring->deq_seg = ring->deq_seg->next;
-   ring->dequeue = ring->deq_seg->trbs;
-   } else {
+   /* event ring doesn't have link trbs, check for last trb */
+   if (ring->type == TYPE_EVENT) {
+   if (!last_trb_on_seg(ring->deq_seg, ring->dequeue)) {
ring->dequeue++;
+   return;
}
-   } while (last_trb(xhci, ring, ring->deq_seg, ring->dequeue));
+   if (last_trb_on_ring(ring, ring->deq_seg, ring->dequeue))
+   ring->cycle_state ^= 1;
+   ring->deq_seg = ring->deq_seg->next;
+   ring->dequeue = ring->deq_seg->trbs;
+   return;
+   }
+
+   /* All other rings have link trbs */
+   if (!trb_is_link(ring->dequeue)) {
+   ring->dequeue++;
+   ring->num_trbs_free++;
+   }
+   while (trb_is_link(ring->dequeue)) {
+   ring->deq_seg = ring->deq_seg->next;
+   ring->dequeue = ring->deq_seg->trbs;
+   }
+   return;
 }
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/14] xhci: remove enqueue_is_link() helper

[Mathias Nyman]

Only used in one place, replace with trb_is_link() helper

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 086b871..2d8dcb1f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -107,12 +107,6 @@ static bool trb_is_link(union xhci_trb *trb)
return TRB_TYPE_LINK_LE32(trb->link.control);
 }
 
-static int enqueue_is_link_trb(struct xhci_ring *ring)
-{
-   struct xhci_link_trb *link = >enqueue->link;
-   return TRB_TYPE_LINK_LE32(link->control);
-}
-
 static bool last_trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb)
 {
return trb == >trbs[TRBS_PER_SEGMENT - 1];
@@ -2873,7 +2867,7 @@ static int prepare_ring(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
}
}
 
-   if (enqueue_is_link_trb(ep_ring)) {
+   if (trb_is_link(ep_ring->enqueue)) {
struct xhci_ring *ring = ep_ring;
union xhci_trb *next;
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] usb: gadget: Allow to build both USB functions and legacy gadgets

[Felipe Balbi]

Krzysztof Opasiak  writes:

> Hi,
>
> On Tue, Jun 7, 2016 at 3:27 AM, Peter Chen  wrote:
>> On Mon, Jun 06, 2016 at 09:40:33PM +0200, Krzysztof Opasiak wrote:
>>> Currently it is possible to build in some subset of usb functions
>>> *OR* some gadget module. This is limited only by Kconfig not
>>> any functionality.
>>>
>>> This patch removes this limitation. With this patch it is possible
>>> to set up all build combinations:
>>> 1) Multiple gadgets build in
>>
>> If that, what the user will expect if choosing multiple gadgets?
>> Eg, if he chooses g_ncm and g_mass_storage, will he expect
>> his udc has both mass_storage and ncm function, but it is
>> not the fact, only the first gadget function will work.
>>
>
> Not exactly one. You may build in multiple modules and use those
> multiple modules if you have multiple udcs.

that's why we introduced configfs, right? That's the only way to be sure
which UDC will bind to which gadget.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Frank Wang]

Hi Heiko,

On 2016/6/20 12:56, Guenter Roeck wrote:

Hi Frank,

On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang  wrote:

Hi Heiko & Guenter,


On 2016/6/20 11:00, Guenter Roeck wrote:

On Sun, Jun 19, 2016 at 6:27 PM, Frank Wang 
wrote:

Hi Guenter,


On 2016/6/17 21:20, Guenter Roeck wrote:

Hi Frank,

On 06/16/2016 11:43 PM, Frank Wang wrote:

Hi Guenter,

On 2016/6/17 12:59, Guenter Roeck wrote:

On 06/16/2016 07:09 PM, Frank Wang wrote:

The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
than rk3288 and before, and most of phy-related registers are also
different from the past, so a new phy driver is required necessarily.

Signed-off-by: Frank Wang 
Suggested-by: Guenter Roeck 
Suggested-by: Doug Anderson 
Reviewed-by: Heiko Stuebner 
Tested-by: Heiko Stuebner 
---


[ ... ]


+
+static int rockchip_usb2phy_resume(struct phy *phy)
+{
+struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+struct rockchip_usb2phy *rphy =
dev_get_drvdata(phy->dev.parent);
+int ret;
+
+dev_dbg(>phy->dev, "port resume\n");
+
+ret = clk_prepare_enable(rphy->clk480m);
+if (ret)
+return ret;
+

If suspend can be called multiple times, resume can be called
multiple times as well. Doesn't this cause a clock imbalance
if you call clk_prepare_enable() multiple times on resume,
but clk_disable_unprepare() only once on suspend ?


Well, what you said is reasonable, How does something like below?

@@ -307,6 +307,9 @@ static int rockchip_usb2phy_resume(struct phy *phy)

   dev_dbg(>phy->dev, "port resume\n");

+   if (!rport->suspended)
+   return 0;
+
   ret = clk_prepare_enable(rphy->clk480m);
   if (ret)
   return ret;
@@ -327,12 +330,16 @@ static int rockchip_usb2phy_suspend(struct phy
*phy)

   dev_dbg(>phy->dev, "port suspend\n");

+   if (rport->suspended)
+   return 0;
+
   ret = property_enable(rphy, >port_cfg->phy_sus, true);
   if (ret)
   return ret;

   rport->suspended = true;
   clk_disable_unprepare(rphy->clk480m);
+
   return 0;
}

@@ -485,6 +492,7 @@ static int rockchip_usb2phy_host_port_init(struct
rockchip_usb2phy *rphy,

   rport->port_id = USB2PHY_PORT_HOST;
   rport->port_cfg =
>phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
+   rport->suspended = true;


Why does it start in suspended mode ? That seems odd.


This is an initialization. Using above design which make 'suspended' as a
condition both in *_usb2phy_resume and *_usb2phy_suspend, I believe if it
is
not initialized as suspended mode, the first resume process will be
skipped.

I had to re-read the entire patch.

Turns out my problem was one of terminology. Using "suspend" and
"resume" to me suggested the common use of suspend and resume
functions. That is not the case here. After mentally replacing
"suspend" with "power_off" and "resume" with "power_on", you are
right, no problem exists. Sorry for the noise.

Maybe it would be useful to replace "resume" with "power_on" and
"suspend" with "power_off" in the function and variable names to
reduce confusion and misunderstandings.

Thanks,
Guenter


Well, it does have a bits confusion, however, the phy-port always just goes
to suspend and resume mode (Not power off and power on) in a fact. So must
it be renamed?


Other phy drivers name the functions _power_off and _power_on and
avoid the confusion. The callbacks are named .power_off and .power_on,
which gives a clear indication of its intended purpose. Other drivers
implementing suspend/resume (such as the omap usb phy driver) tie
those functions not into the power_off/power_on callbacks, but into
the driver's suspend/resume callbacks. At least the omap driver has
separate power management functions.

Do the functions _have_ to be renamed ? Surely not. But, if the
functions are really suspend/resume functions and not
power_off/power_on functions, maybe they should tie to the
suspend/resume functions and not register themselves as
power_off/power_on functions ?

Thanks,
Guenter


As Guenter mentioned above,  I doped out two solutions, one is that keep 
current process but renaming *_resume/*_suspend to 
*_power_on/*_power_off; another is that do not assign power_on/power_off 
functions for phy_ops at phy creating time, then, shorten 
_SCHEDULE_DELAY_ delay time less that 10 Seconds, and the phy-port 
suspend/resume mechanism depend on _sm_work_ completely.


So which is the better way from your view? or would you like to give 
other unique perceptions please?


BR.
Frank




Theoretically, the phy-port in suspended mode make sense when it is at
start
time, then the upper layer controller will invoke phy_power_on (See
phy-core.c), and it further call back *_usb2phy_resume to make phy-port
work
properly.

Re: [PATCH v3 00/13] usb: dwc2: Fix up gadget isochronous support

[Felipe Balbi]

Hi,

John Youn  writes:
> On 5/25/2016 6:06 PM, John Youn wrote:
>> The following patch series fixes up isochronous support for the dwc2
>> gadget. The existing isochronous support lacked a few features. Most
>> notably it did not properly sync up with the first packet and it
>> didn't handle the Incomplete ISO IN/OUT interrupts.
>> 
>> These patches have been sitting in our internal tree for a while and
>> tested mostly on a HAPS-based FPGA IP validation platform.
>> 
>> v3:
>> * Simplified setting endpoint interval in patch 7/13
>> 
>> v2:
>> * Changed if/else to switch statement
>> * Changed frame_overrun to bool
>> * Downgraded a info message to dbg
>> * Added John Keeping's tested-by
>> 
>> Regards,
>> John
>> 
>> 
>> Vardan Mikayelyan (13):
>>   usb: dwc2: Add missing register field definitions
>>   usb: dwc2: gadget: Remove unnecessary line
>>   usb: dwc2: gadget: Remove unnecessary code
>>   usb: dwc2: gadget: Corrected field names
>>   usb: dwc2: gadget: Fix transfer stop programming for out endpoint
>>   usb: dwc2: gadget: Add dwc2_gadget_incr_frame_num()
>>   usb: dwc2: gadget: Corrected interval calculation
>>   usb: dwc2: gadget: Add dwc2_gadget_read_ep_interrupts function
>>   usb: dwc2: gadget: Add dwc2_gadget_start_next_request function
>>   usb: dwc2: gadget: Add OUTTKNEPDIS and NAKINTRPT handlers
>>   usb: dwc2: gadget: Add Incomplete ISO IN/OUT Interrupt handlers
>>   usb: dwc2: gadget: Add EP disabled interrupt handler
>>   usb: dwc2: gadget: Final fixes for BDMA ISOC
>> 
>>  drivers/usb/dwc2/core.h   |   8 +-
>>  drivers/usb/dwc2/gadget.c | 574 
>> --
>>  drivers/usb/dwc2/hw.h |  14 ++
>>  3 files changed, 470 insertions(+), 126 deletions(-)
>> 
>
>
> Hi Felipe,
>
> Could you take this series in your testing/next branch?
>
> I'd like it to have as much testing as possible.

sorry for the long delay, It's there now.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 06/14] xhci: TD-fragment, align the unsplittable case with a bounce buffer

[Mathias Nyman]

If the last trb before a link is not packet size aligned, and is not
splittable then use a bounce buffer for that chunk of max packet size
unalignable data.

Allocate a max packet size bounce buffer for every segment of a bulk
endpoint ring at the same time as allocating the ring.
If we need to align the data before the link trb in that segment then
copy the data to the segment bounce buffer, dma map it, and enqueue it.
Once the td finishes, or is cancelled, unmap it.

For in transfers we need to first map the bounce buffer, then queue it,
after it finishes, copy the bounce buffer to the original sg list, and
finally unmap it

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c  |  74 ++
 drivers/usb/host/xhci-ring.c | 106 +++
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  10 +++-
 4 files changed, 155 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bad0d1f..6afe323 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -37,7 +37,9 @@
  * "All components of all Command and Transfer TRBs shall be initialized to 
'0'"
  */
 static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
-   unsigned int cycle_state, gfp_t flags)
+  unsigned int cycle_state,
+  unsigned int max_packet,
+  gfp_t flags)
 {
struct xhci_segment *seg;
dma_addr_t  dma;
@@ -53,6 +55,14 @@ static struct xhci_segment *xhci_segment_alloc(struct 
xhci_hcd *xhci,
return NULL;
}
 
+   if (max_packet) {
+   seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
+   if (!seg->bounce_buf) {
+   dma_pool_free(xhci->segment_pool, seg->trbs, dma);
+   kfree(seg);
+   return NULL;
+   }
+   }
/* If the cycle state is 0, set the cycle bit to 1 for all the TRBs */
if (cycle_state == 0) {
for (i = 0; i < TRBS_PER_SEGMENT; i++)
@@ -70,6 +80,7 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct 
xhci_segment *seg)
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
seg->trbs = NULL;
}
+   kfree(seg->bounce_buf);
kfree(seg);
 }
 
@@ -317,11 +328,11 @@ static void xhci_initialize_ring_info(struct xhci_ring 
*ring,
 static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment **first, struct xhci_segment **last,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_segment *prev;
 
-   prev = xhci_segment_alloc(xhci, cycle_state, flags);
+   prev = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!prev)
return -ENOMEM;
num_segs--;
@@ -330,7 +341,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
while (num_segs > 0) {
struct xhci_segment *next;
 
-   next = xhci_segment_alloc(xhci, cycle_state, flags);
+   next = xhci_segment_alloc(xhci, cycle_state, max_packet, flags);
if (!next) {
prev = *first;
while (prev) {
@@ -360,7 +371,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd 
*xhci,
  */
 static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
-   enum xhci_ring_type type, gfp_t flags)
+   enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
 {
struct xhci_ring*ring;
int ret;
@@ -370,13 +381,15 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
*xhci,
return NULL;
 
ring->num_segs = num_segs;
+   ring->bounce_buf_len = max_packet;
INIT_LIST_HEAD(>td_list);
ring->type = type;
if (num_segs == 0)
return ring;
 
ret = xhci_alloc_segments_for_ring(xhci, >first_seg,
-   >last_seg, num_segs, cycle_state, type, flags);
+   >last_seg, num_segs, cycle_state, type,
+   max_packet, flags);
if (ret)
goto fail;
 
@@ -470,7 +483,8 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
ring->num_segs : num_segs_needed;
 
ret = xhci_alloc_segments_for_ring(xhci, , ,
-   num_segs, ring->cycle_state, ring->type, flags);
+   num_segs, ring->cycle_state, 

[PATCH 08/14] xhci: use and add separate function for checking for link trbs

[Mathias Nyman]

Add a new is_link_trb() function that only checks for link trbs.
We want to split generic last_trb() function which is used for both
event rings without link trbs, and endpoint and command rings with links.

This will allow us to easier check for link trbs added mid segments.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c13b842..4de8a2b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -115,6 +115,11 @@ static int last_trb(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
return TRB_TYPE_LINK_LE32(trb->link.control);
 }
 
+static bool trb_is_link(union xhci_trb *trb)
+{
+   return TRB_TYPE_LINK_LE32(trb->link.control);
+}
+
 static int enqueue_is_link_trb(struct xhci_ring *ring)
 {
struct xhci_link_trb *link = >enqueue->link;
@@ -130,7 +135,7 @@ static void next_trb(struct xhci_hcd *xhci,
struct xhci_segment **seg,
union xhci_trb **trb)
 {
-   if (last_trb(xhci, ring, *seg, *trb)) {
+   if (trb_is_link(*trb)) {
*seg = (*seg)->next;
*trb = ((*seg)->trbs);
} else {
@@ -150,8 +155,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring 
*ring)
 * If this is not event ring, and the dequeue pointer
 * is not on a link TRB, there is one more usable TRB
 */
-   if (ring->type != TYPE_EVENT &&
-   !last_trb(xhci, ring, ring->deq_seg, ring->dequeue))
+   if (ring->type != TYPE_EVENT && !trb_is_link(ring->dequeue))
ring->num_trbs_free++;
 
do {
@@ -199,13 +203,13 @@ static void inc_enq(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
 
chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN;
/* If this is not event ring, there is one less usable TRB */
-   if (!last_trb(xhci, ring, ring->enq_seg, ring->enqueue))
+   if (!trb_is_link(ring->enqueue))
ring->num_trbs_free--;
next = ++(ring->enqueue);
 
ring->enq_updates++;
/* Update the dequeue pointer further if that was a link TRB */
-   while (last_trb(xhci, ring, ring->enq_seg, next)) {
+   while (trb_is_link(next)) {
 
/*
 * If the caller doesn't plan on enqueueing more TDs before
@@ -931,7 +935,7 @@ static void update_ring_for_set_deq_completion(struct 
xhci_hcd *xhci,
 * the dequeue pointer one segment further, or we'll jump off
 * the segment into la-la-land.
 */
-   if (last_trb(xhci, ep_ring, ep_ring->deq_seg, ep_ring->dequeue)) {
+   if (trb_is_link(ep_ring->dequeue)) {
ep_ring->deq_seg = ep_ring->deq_seg->next;
ep_ring->dequeue = ep_ring->deq_seg->trbs;
}
@@ -940,8 +944,7 @@ static void update_ring_for_set_deq_completion(struct 
xhci_hcd *xhci,
/* We have more usable TRBs */
ep_ring->num_trbs_free++;
ep_ring->dequeue++;
-   if (last_trb(xhci, ep_ring, ep_ring->deq_seg,
-   ep_ring->dequeue)) {
+   if (trb_is_link(ep_ring->dequeue)) {
if (ep_ring->dequeue ==
dev->eps[ep_index].queued_deq_ptr)
break;
@@ -2880,7 +2883,7 @@ static int prepare_ring(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
 
next = ring->enqueue;
 
-   while (last_trb(xhci, ring, ring->enq_seg, next)) {
+   while (trb_is_link(next)) {
/* If we're not dealing with 0.95 hardware or isoc rings
 * on AMD 0.96 host, clear the chain bit.
 */
@@ -3269,8 +3272,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 */
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
-   if (last_trb(xhci, ring, ring->enq_seg,
-ring->enqueue + 1)) {
+   if (trb_is_link(ring->enqueue + 1)) {
if (xhci_align_td(xhci, urb, enqd_len,
  _buff_len,
  ring->enq_seg)) {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/14] xhci: clean up event ring checks from inc_enq()

[Mathias Nyman]

Remove the event ring related checks in inc_enq()

Host hardware is the producer of events on the event ring,
driver will not queue anything, or call inc_enq() for the
event ring.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 64 +++-
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 142e53e..c13b842 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -199,50 +199,42 @@ static void inc_enq(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
 
chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN;
/* If this is not event ring, there is one less usable TRB */
-   if (ring->type != TYPE_EVENT &&
-   !last_trb(xhci, ring, ring->enq_seg, ring->enqueue))
+   if (!last_trb(xhci, ring, ring->enq_seg, ring->enqueue))
ring->num_trbs_free--;
next = ++(ring->enqueue);
 
ring->enq_updates++;
-   /* Update the dequeue pointer further if that was a link TRB or we're at
-* the end of an event ring segment (which doesn't have link TRBS)
-*/
+   /* Update the dequeue pointer further if that was a link TRB */
while (last_trb(xhci, ring, ring->enq_seg, next)) {
-   if (ring->type != TYPE_EVENT) {
-   /*
-* If the caller doesn't plan on enqueueing more
-* TDs before ringing the doorbell, then we
-* don't want to give the link TRB to the
-* hardware just yet.  We'll give the link TRB
-* back in prepare_ring() just before we enqueue
-* the TD at the top of the ring.
-*/
-   if (!chain && !more_trbs_coming)
-   break;
 
-   /* If we're not dealing with 0.95 hardware or
-* isoc rings on AMD 0.96 host,
-* carry over the chain bit of the previous TRB
-* (which may mean the chain bit is cleared).
-*/
-   if (!(ring->type == TYPE_ISOC &&
-   (xhci->quirks & XHCI_AMD_0x96_HOST))
-   && !xhci_link_trb_quirk(xhci)) {
-   next->link.control &=
-   cpu_to_le32(~TRB_CHAIN);
-   next->link.control |=
-   cpu_to_le32(chain);
-   }
-   /* Give this link TRB to the hardware */
-   wmb();
-   next->link.control ^= cpu_to_le32(TRB_CYCLE);
+   /*
+* If the caller doesn't plan on enqueueing more TDs before
+* ringing the doorbell, then we don't want to give the link TRB
+* to the hardware just yet. We'll give the link TRB back in
+* prepare_ring() just before we enqueue the TD at the top of
+* the ring.
+*/
+   if (!chain && !more_trbs_coming)
+   break;
 
-   /* Toggle the cycle bit after the last ring segment. */
-   if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, 
next)) {
-   ring->cycle_state ^= 1;
-   }
+   /* If we're not dealing with 0.95 hardware or isoc rings on
+* AMD 0.96 host, carry over the chain bit of the previous TRB
+* (which may mean the chain bit is cleared).
+*/
+   if (!(ring->type == TYPE_ISOC &&
+ (xhci->quirks & XHCI_AMD_0x96_HOST)) &&
+   !xhci_link_trb_quirk(xhci)) {
+   next->link.control &= cpu_to_le32(~TRB_CHAIN);
+   next->link.control |= cpu_to_le32(chain);
}
+   /* Give this link TRB to the hardware */
+   wmb();
+   next->link.control ^= cpu_to_le32(TRB_CYCLE);
+
+   /* Toggle the cycle bit after the last ring segment. */
+   if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next))
+   ring->cycle_state ^= 1;
+
ring->enq_seg = ring->enq_seg->next;
ring->enqueue = ring->enq_seg->trbs;
next = ring->enqueue;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/14] xhci: rename ep_ring variable in queue_bulk_tx(), no functional change

[Mathias Nyman]

Tiny change, a bit more readable.
The real reason for this change is that the coming td fragment work
had several over 80 lines character lines split just because of a few
extra characters in variable names.

no functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 52deae4..c3e9a60 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3102,7 +3102,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
 {
-   struct xhci_ring *ep_ring;
+   struct xhci_ring *ring;
struct urb_priv *urb_priv;
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
@@ -3111,14 +3111,13 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
bool zero_length_needed;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len;
-   unsigned int full_len;
+   unsigned int running_total, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
 
-   ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
-   if (!ep_ring)
+   ring = xhci_urb_to_transfer_ring(xhci, urb);
+   if (!ring)
return -EINVAL;
 
/* If we have scatter/gather list, we use it. */
@@ -3159,8 +3158,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 * until we've finished creating all the other TRBs.  The ring's cycle
 * state may change as we enqueue the other TRBs, so save it too.
 */
-   start_trb = _ring->enqueue->generic;
-   start_cycle = ep_ring->cycle_state;
+   start_trb = >enqueue->generic;
+   start_cycle = ring->cycle_state;
 
full_len = urb->transfer_buffer_length;
running_total = 0;
@@ -3199,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
-   field |= ep_ring->cycle_state;
+   field |= ring->cycle_state;
 
/* Chain all the TRBs together; clear the chain bit in the last
 * TRB to indicate it's the last TRB in the chain.
@@ -3209,10 +3208,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
} else {
field |= TRB_IOC;
if (i == last_trb_num)
-   td->last_trb = ep_ring->enqueue;
+   td->last_trb = ring->enqueue;
else if (zero_length_needed) {
trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ep_ring->enqueue;
+   urb_priv->td[1]->last_trb = ring->enqueue;
}
}
 
@@ -3233,7 +3232,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
more_trbs_coming = true;
else
more_trbs_coming = false;
-   queue_trb(xhci, ep_ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/14] xhci: don't rely on precalculated value of needed trbs in the enqueue loop

[Mathias Nyman]

Queue trbs until all payload data in the urb is tranferred.

The actual number of trbs might need to change from the pre-calculated
number when the packet alignment restrictions for td fragments in
xhci 4.11.7.1 are taken into account.

Long term plan is to get rid of calculating the needed trbs in advance
all together. It's an unnecessary extra walk through the scatterlist.

This change also allows some bulk queue function simplifications

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 75 +---
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f74ac1c..d86da81 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3109,9 +3109,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct scatterlist *sg = NULL;
bool more_trbs_coming = true;
bool need_zero_pkt = false;
-   unsigned int num_trbs, last_trb_num, i;
+   bool first_trb = true;
+   unsigned int num_trbs;
unsigned int start_cycle, num_sgs = 0;
-   unsigned int running_total, block_len, trb_buff_len, full_len;
+   unsigned int enqd_len, block_len, trb_buff_len, full_len;
int ret;
u32 field, length_field, remainder;
u64 addr;
@@ -3120,14 +3121,19 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (!ring)
return -EINVAL;
 
+   full_len = urb->transfer_buffer_length;
/* If we have scatter/gather list, we use it. */
if (urb->num_sgs) {
num_sgs = urb->num_mapped_sgs;
sg = urb->sg;
+   addr = (u64) sg_dma_address(sg);
+   block_len = sg_dma_len(sg);
num_trbs = count_sg_trbs_needed(urb);
-   } else
+   } else {
num_trbs = count_trbs_needed(urb);
-
+   addr = (u64) urb->transfer_dma;
+   block_len = full_len;
+   }
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
@@ -3136,8 +3142,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 
urb_priv = urb->hcpriv;
 
-   last_trb_num = num_trbs - 1;
-
/* Deal with URB_ZERO_PACKET - need one more td/trb */
if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
need_zero_pkt = true;
@@ -3152,40 +3156,20 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
start_trb = >enqueue->generic;
start_cycle = ring->cycle_state;
 
-   full_len = urb->transfer_buffer_length;
-   running_total = 0;
-   block_len = 0;
-
/* Queue the TRBs, even if they are zero-length */
-   for (i = 0; i < num_trbs; i++) {
+   for (enqd_len = 0; enqd_len < full_len; enqd_len += trb_buff_len) {
field = TRB_TYPE(TRB_NORMAL);
 
-   if (block_len == 0) {
-   /* A new contiguous block. */
-   if (sg) {
-   addr = (u64) sg_dma_address(sg);
-   block_len = sg_dma_len(sg);
-   } else {
-   addr = (u64) urb->transfer_dma;
-   block_len = full_len;
-   }
-   /* TRB buffer should not cross 64KB boundaries */
-   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
-   trb_buff_len = min_t(unsigned int,
-   trb_buff_len,
-   block_len);
-   } else {
-   /* Further through the contiguous block. */
-   trb_buff_len = block_len;
-   if (trb_buff_len > TRB_MAX_BUFF_SIZE)
-   trb_buff_len = TRB_MAX_BUFF_SIZE;
-   }
+   /* TRB buffer should not cross 64KB boundaries */
+   trb_buff_len = TRB_BUFF_LEN_UP_TO_BOUNDARY(addr);
+   trb_buff_len = min_t(unsigned int, trb_buff_len, block_len);
 
-   if (running_total + trb_buff_len > full_len)
-   trb_buff_len = full_len - running_total;
+   if (enqd_len + trb_buff_len > full_len)
+   trb_buff_len = full_len - enqd_len;
 
/* Don't change the cycle bit of the first TRB until later */
-   if (i == 0) {
+   if (first_trb) {
+   first_trb = false;
if (start_cycle == 0)
field |= TRB_CYCLE;
} else
@@ -3194,7 +3178,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
   

[PATCH 05/14] xhci: align the last trb before link if it is easily splittable.

[Mathias Nyman]

TD fragments section 4.11.7.1 in xhci specs have additional requirements
on how trbs in TDs must be organized.

TD fragments shall not span transfer ring segments and TD fragments must
be packet aligned. Normally we don't care about TD fragments, on TD is one
big fragment, but if a TD spans ring segments it will be treated as two
fragments, and we need to comply with the alignment requirements.

For us this means that the payload data must be packet aligned in the
last trb before a link trb.
In most mass storage bulk tranfers we are lucky as the block size aligns
nicely with packet size, and there are no issues.
However, usb network adapters using scatterlists can hit this alignment
issue, and usbtest in kernel triggers this in minutes.

This patch is a partial solution, it solves the easy case when the last
trb before the link trb contains a packet boundary.
If that is the case then just split the trb at the boundary.
If not, then just print a debug message and continue as we have always
done, hoping for the best

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d86da81..bf9ffa4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,6 +3098,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
 }
 
+static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
+u32 *trb_buff_len)
+{
+   unsigned int unalign;
+   unsigned int max_pkt;
+
+   max_pkt = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
+   unalign = (enqd_len + *trb_buff_len) % max_pkt;
+
+   /* we got lucky, last normal TRB data on segment is packet aligned */
+   if (unalign == 0)
+   return 0;
+
+   /* is the last nornal TRB alignable by splitting it */
+   if (*trb_buff_len > unalign) {
+   *trb_buff_len -= unalign;
+   return 0;
+   }
+   return 1;
+}
+
 /* This is very similar to what ehci-q.c qtd_fill() does */
 int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
@@ -3180,6 +3201,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
 */
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
+   if (last_trb(xhci, ring, ring->enq_seg,
+ring->enqueue + 1)) {
+   if (xhci_align_td(xhci, urb, enqd_len,
+_buff_len))
+   xhci_dbg(xhci, "TRB align fail\n");
+   }
} else {
field |= TRB_IOC;
more_trbs_coming = false;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/14] xhci: properly prepare zero packet TD after normal bulk TD.

[Mathias Nyman]

If a zero-length packet is needed after a bulk transfer, then an
additional zero length TD was prepared before enqueueing the bulk transfer
This set up the zero packet TD structure with incorrect td->start_seg
and td->first_trb pointers.

Prepare the zero packet TD after the data bulk TD is enqueued instead.
It sets these pointers correctly.

This change also simplifies unnecessary complexity related to keeping
track of the last trb when enqueuing trbs.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c3e9a60..15dd226 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3107,8 +3107,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
struct xhci_td *td;
struct xhci_generic_trb *start_trb;
struct scatterlist *sg = NULL;
-   bool more_trbs_coming;
-   bool zero_length_needed;
+   bool more_trbs_coming = true;
+   bool need_zero_pkt = false;
unsigned int num_trbs, last_trb_num, i;
unsigned int start_cycle, num_sgs = 0;
unsigned int running_total, block_len, trb_buff_len, full_len;
@@ -3139,17 +3139,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
last_trb_num = num_trbs - 1;
 
/* Deal with URB_ZERO_PACKET - need one more td/trb */
-   zero_length_needed = urb->transfer_flags & URB_ZERO_PACKET &&
-   urb_priv->length == 2;
-   if (zero_length_needed) {
-   num_trbs++;
-   xhci_dbg(xhci, "Creating zero length td.\n");
-   ret = prepare_transfer(xhci, xhci->devs[slot_id],
-   ep_index, urb->stream_id,
-   1, urb, 1, mem_flags);
-   if (unlikely(ret < 0))
-   return ret;
-   }
+   if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->length > 1)
+   need_zero_pkt = true;
 
td = urb_priv->td[0];
 
@@ -3207,12 +3198,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   if (i == last_trb_num)
-   td->last_trb = ring->enqueue;
-   else if (zero_length_needed) {
-   trb_buff_len = 0;
-   urb_priv->td[1]->last_trb = ring->enqueue;
-   }
+   more_trbs_coming = need_zero_pkt;
+   td->last_trb = ring->enqueue;
}
 
/* Only set interrupt on short packet for IN endpoints */
@@ -3228,10 +3215,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   if (i < num_trbs - 1)
-   more_trbs_coming = true;
-   else
-   more_trbs_coming = false;
queue_trb(xhci, ring, more_trbs_coming,
lower_32_bits(addr),
upper_32_bits(addr),
@@ -3253,6 +3236,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
}
}
 
+   if (need_zero_pkt) {
+   ret = prepare_transfer(xhci, xhci->devs[slot_id],
+  ep_index, urb->stream_id,
+  1, urb, 1, mem_flags);
+   urb_priv->td[1]->last_trb = ring->enqueue;
+   field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
+   queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
+   }
+
check_trb_math(urb, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
start_cycle, start_trb);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/14] xhci: use boolean to indicate last trb in td remainder calculation

[Mathias Nyman]

We only need to know if we are queuing the last trb for a TD when
calculating the td remainder field.
The total number of trbs left is not used.

We won't be able to trust the pre-calculated number of trbs used if we
need to align trb data by splitting or merging trbs in order to satisfy
comply with data alignment requirements in xhci specs section 4.11.7.1.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 15dd226..f74ac1c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3074,7 +3074,7 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
  */
 static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
  int trb_buff_len, unsigned int td_total_len,
- struct urb *urb, unsigned int num_trbs_left)
+ struct urb *urb, bool more_trbs_coming)
 {
u32 maxp, total_packet_count;
 
@@ -3083,7 +3083,7 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
return ((td_total_len - transferred) >> 10);
 
/* One TRB with a zero-length data packet. */
-   if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
+   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
return 0;
 
@@ -3198,7 +3198,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
field |= TRB_CHAIN;
} else {
field |= TRB_IOC;
-   more_trbs_coming = need_zero_pkt;
+   more_trbs_coming = false;
td->last_trb = ring->enqueue;
}
 
@@ -3209,13 +3209,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
/* Set the TRB length, TD size, and interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
trb_buff_len, full_len,
-   urb, num_trbs - i - 1);
-
+   urb, more_trbs_coming);
length_field = TRB_LEN(trb_buff_len) |
TRB_TD_SIZE(remainder) |
TRB_INTR_TARGET(0);
 
-   queue_trb(xhci, ring, more_trbs_coming,
+   queue_trb(xhci, ring, more_trbs_coming | need_zero_pkt,
lower_32_bits(addr),
upper_32_bits(addr),
length_field,
@@ -3639,7 +3638,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, 
gfp_t mem_flags,
/* Set the TRB length, TD size, & interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
   trb_buff_len, td_len,
-  urb, trbs_per_td - j - 1);
+  urb, more_trbs_coming);
 
length_field = TRB_LEN(trb_buff_len) |
TRB_INTR_TARGET(0);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/14] xhci features for usb-next

[Mathias Nyman]

Hi Greg

These patches for 4.8 contain td-fragment work that ensures bulk tranfers
finally follows the aligment requirements in the xhci specs.

Also reworking helper functions for checking last and link trbs,
and cleanups for platform data

-Mathias

Heikki Krogerus (3):
  xhci: plat: adapt to unified device property interface
  usb: dwc3: host: use build-in property instead of platform data
  xhci: get rid of platform data

Mathias Nyman (11):
  xhci: rename ep_ring variable in queue_bulk_tx(), no functional change
  xhci: properly prepare zero packet TD after normal bulk TD.
  xhci: use boolean to indicate last trb in td remainder calculation
  xhci: don't rely on precalculated value of needed trbs in the enqueue
loop
  xhci: align the last trb before link if it is easily splittable.
  xhci: TD-fragment, align the unsplittable case with a bounce buffer
  xhci: clean up event ring checks from inc_enq()
  xhci: use and add separate function for checking for link trbs
  xhci: rework inc_deq() and fix off by one error.
  xhci: remove enqueue_is_link() helper
  xhci: rename and simplify last_trb_on_last_seg() helper

 drivers/usb/dwc3/host.c  |  18 +-
 drivers/usb/host/xhci-mem.c  |  74 ---
 drivers/usb/host/xhci-plat.c |   8 +-
 drivers/usb/host/xhci-ring.c | 457 ++-
 drivers/usb/host/xhci.c  |   5 +-
 drivers/usb/host/xhci.h  |  10 +-
 include/linux/usb/xhci_pdriver.h |  27 ---
 7 files changed, 326 insertions(+), 273 deletions(-)
 delete mode 100644 include/linux/usb/xhci_pdriver.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v11 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi Baolin,

Baolin Wang  writes:
>>> -Original Message-
>>> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
>>> ow...@vger.kernel.org] On Behalf Of Baolin Wang
>>> Sent: Monday, June 13, 2016 4:47 PM
>>> To: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
>>> dbarysh...@gmail.com; dw...@infradead.org
>>> Cc: r...@kernel.org; m.szyprow...@samsung.com; ruslan.bilo...@gmail.com;
>>> peter.c...@freescale.com; st...@rowland.harvard.edu; r.bald...@samsung.com;
>>> yoshihiro.shimoda...@renesas.com; lee.jo...@linaro.org; broo...@kernel.org;
>>> ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
>>> baolin.w...@linaro.org; linux...@vger.kernel.org; linux-
>>> u...@vger.kernel.org; device-mainlin...@lists.linuxfoundation.org; linux-
>>> ker...@vger.kernel.org
>>> Subject: [RESEND PATCH v11 1/4] gadget: Introduce the usb charger
>>> framework
>>>
>>> This patch introduces the usb charger driver based on usb gadget that
>>> makes an enhancement to a power driver. It works well in practice but that
>>> requires a system with suitable hardware.
>>>
>>> The basic conception of the usb charger is that, when one usb charger is
>>> added or removed by reporting from the usb gadget state change or the
>>> extcon device state change, the usb charger will report to power user to
>>> set the current limitation.
>>>
>>> The usb charger will register notifiees on the usb gadget or the extcon
>>> device to get notified the usb charger state. It also supplies the
>>> notification mechanism to userspace When the usb charger state is changed.
>>>
>>> Power user will register a notifiee on the usb charger to get notified by
>>> status changes from the usb charger. It will report to power user to set
>>> the current limitation when detecting the usb charger is added or removed
>>> from extcon device state or usb gadget state.
>>>
>>> This patch doesn't yet integrate with the gadget code, so some functions
>>> which rely on the 'gadget' are not completed, that will be implemented in
>>> the following patches.
>>>
>>> Signed-off-by: Baolin Wang 
>>
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>
> Do you have any comments about this patchset, if not I will resend it
> with testing by Li Jun (thanks for Jun reviewing and testing). Thanks.

please resend. I don't have the series in my queue anymore, sorry.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: dwc3: gadget: issue ENDTRANSFER conditional on resource_index

[Felipe Balbi]

Because of recent changes to transfer handling on
DWC3, we will not get XferComplete unless we
completely fill up our TRB ring. This means that we
might get a Reset or Disconnect without getting a
XferComplete first.

In order to correctly release our allocated Transfer
Resource, we must issue ENDTRANSFER command whenever
dep->resource_index is valid.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 20 +---
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0afaa9d58562..b1fec36f4764 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -607,24 +607,14 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, 
u32 epnum, bool force);
 static void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
 {
struct dwc3_request *req;
-   struct dwc3_trb *current_trb;
-   unsignedtransfer_in_flight;
 
-   if (dep->number > 1)
-   current_trb = >trb_pool[dep->trb_enqueue];
-   else
-   current_trb = >ep0_trb[dep->trb_enqueue];
-   transfer_in_flight = current_trb->ctrl & DWC3_TRB_CTRL_HWO;
-
-   if (transfer_in_flight && !list_empty(>started_list)) {
-   dwc3_stop_active_transfer(dwc, dep->number, true);
+   dwc3_stop_active_transfer(dwc, dep->number, true);
 
-   /* - giveback all requests to gadget driver */
-   while (!list_empty(>started_list)) {
-   req = next_request(>started_list);
+   /* - giveback all requests to gadget driver */
+   while (!list_empty(>started_list)) {
+   req = next_request(>started_list);
 
-   dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
-   }
+   dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
}
 
while (!list_empty(>pending_list)) {
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> Hi Roger,
>
>> From: Roger Quadros
>> Sent: Monday, June 20, 2016 7:13 PM
>> 
>> Hi,
>> 
>> On 20/06/16 10:45, Felipe Balbi wrote:
> < snip >
>> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> >> index f4fc0aa..1d74fb8 100644
>> >> --- a/include/linux/usb/gadget.h
>> >> +++ b/include/linux/usb/gadget.h
>> >> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>> >>   * @in_epnum: last used in ep number
>> >>   * @mA: last set mA value
>> >>   * @otg_caps: OTG capabilities of this gadget.
>> >> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >
>> > do you really know of any platform which has a separate OTG controller?
>> >
>> 
>> Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> and gadget.
>> 
>> [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> 
>> Yoshihiro,
>> 
>> How is the dual-role architecture on your Renesas platform?
>
> About the dual-role architecture, Renesas platform (R-Car H3) has a
> USB 2.0 host controller (EHCI/OHCI) with OTG function and a separate
> USB 2.0 peripheral controller (HS-USB).  The OTG function is related
> to some PHY control registers, so I intend to add the OTG/Dual-role
> core support into the phy driver (drivers/phy/phy-rcar-gen3-usb2.c).

that looks like a mux to me :-) thanks for the pointer

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >> > It provides APIs for the following tasks
>> >> >
>> >> > - Registering an OTG/dual-role capable controller
>> >> > - Registering Host and Gadget controllers to OTG core
>> >> > - Providing inputs to and kicking the OTG state machine
>> >> 
>> >> I think I have already mentioned this, but after over 10 years of OTG,
>> >> nobody seems to care about it, why are we still touching at all I don't
>> >> know. For common non-OTG role-swapping we really don't need any of this
>> >> and, quite frankly, I fail to see enough users for this.
>> >> 
>> >> Apparently there's only chipidea which, AFAICT, already had working
>> >> dual-role before this OTG State Machine was added to the kernel.
>> >
>> > Some users would like to know if vendor's platform is OTG compliance,
>> > so we add it to pass usb.org USB OTG certification test.
>> 
>> I strongly doubt that's really what they mean. IMHO, users want to know
>> if they can swap roles. Ask them if they are really going for OTG
>> certification. Ask them if they have an OPT tester. Ask them if they
>> really want all those timers. If they want HNP polling, etc etc etc.
>> 
>> So far, I haven't seen anybody talking about real USB OTG (the spec)
>> when they say OTG. Usually they just mean "a method for swapping between
>> host and peripheral roles, but we really don't want all the extra cost
>> of the OTG specification".
>> 
>
> That's what I thought before, but the request from the Marketing guy is
> "To prove the SoC is OTG compliance, support HNP and SRP", don't you
> see the SoC reference manual say "it supports HNP and SRP"?
>
> If there is no request, who else wants to implement so complicated FSM
> but seldom use cases, and go to pass OTG compliance test (tested by PET).

I stand corrected :-)

So there is one user for this layer. And this user has its own role
control registers. I'm not convinced we need this large generic layer
for one user.

>> > For the real use case, some Carplay platforms need it.
>> 
>> Carplay does *NOT* rely on OTG. Apple has its own proprietary and closed
>> specification which is not OTG-compliant.
>> 
>
> Yes, it is not OTG-compliant, but it can co-work with some standard OTG FSM
> states to finish role swap.

What are you referring to as "finish role swap"? I don't get that.

> Notice, it needs to swap role without disconnect cable.

right, I can swap role without changing cable, but that's not OTG. The
mechanism for that, AFAICT, is not HNP. I don't know details about
CarPlay because the spec isn't public, but my understanding is that
CarPlay doesn't rely on anything from OTG spec.

>> >> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> >> > index f4fc0aa..1d74fb8 100644
>> >> > --- a/include/linux/usb/gadget.h
>> >> > +++ b/include/linux/usb/gadget.h
>> >> > @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>> >> >   * @in_epnum: last used in ep number
>> >> >   * @mA: last set mA value
>> >> >   * @otg_caps: OTG capabilities of this gadget.
>> >> > + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> 
>> >> do you really know of any platform which has a separate OTG controller?
>> >> 
>> >
>> > It may not be a real separate OTG controller. It can be a hardware part
>> > (external connector, external IC, SoC OTG register area, etc) to handle 
>> > vbus
>> > ,id and other signals which are used for role swap.
>> 
>> That's already solved. EXTCON solved that years back and OMAP has been
>> using EXTCON to program its UTMI mailbox.
>> 
>
> No, that's not the same thing, it does not include the swap role.

Read your original comment:

"handle vbus, id and other signals which are *used for* role swap"

You didn't include role swap in your original comment. Semantics aside...

> Consider the use case the host driver is at host/ and udc driver is
> at gadget/udc, how to finish to role swap?

... why does the source code placement matter? And what do you mean by
"finish role swap"?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >>> +
>> >>> +/* start host */
>> >>> +ret = hcd_ops->add(otg->primary_hcd.hcd,
>> >>> +   otg->primary_hcd.irqnum,
>> >>> +   otg->primary_hcd.irqflags);
>> >> 
>> >> this is usb_add_hcd(), is it not? Why add an indirection?
>> >
>> > I've introduced the host and gadget ops interface to get around the
>> > circular dependency issue we can't avoid.
>> > otg needs to call host/gadget functions and host/gadget also needs to
>> > call otg functions.
>> 
>> IMO, this shows a fragility of your design. You're, now, lying to
>> usb_hcd and usb_udc and making them register into a virtual layer that
>> doesn't exist. And that layer will end up calling the real registration
>> function when some magic event happens.
>> 
>> This is only really needed for quirky devices like dwc3 (but see more on
>> dwc3 below) where host and peripheral registers shadow each
>> other. Otherwise we would be able to always keep hcd and udc always
>> registered. They would get different interrupt statuses anyway and
>> nothing would ever break.
>> 
>> However, when it comes to dwc3, we already have all the code necessary
>> to workaround this issue by destroying the XHCI pdev when OTG interrupt
>> says we should be peripheral (and vice-versa). DWC3 also keeps track of
>> the OTG states for those folks who really care about OTG (Hint: nobody
>> has cared for the past 10 years, why would they do so now?) and we don't
>> need a SW state machine when the HW handles that for us, right?
>> 
>> As for chipidea, IIRC, that doesn't need a SW state machine either, but
>> I know very little about that IP and don't even have documentation on
>> it. My understanding, however, is that chipidea behaves kinda like MUSB,
>> which changes roles automatically in HW based on ID pin state.
>
> Chipidea needs to set register for USB role manually.

okay, so chipidea has private control of role. Much like dwc3. That's good.

>> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> 
>> >> do you really know of any platform which has a separate OTG controller?
>> >> 
>> >
>> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, 
>> > host
>> > and gadget.
>> >
>> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> 
>> that's not an OTG controller, it's just a mux. No different than Intel's
>> mux for swapping between XHCI and peripheral-only DWC3.
>> 
>> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> are two competing standards and, apparently, type-C is winning when it
>> comes to role-swapping.
>> 
>
> In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> It will be considered as an OTG port.

That's because "dual-role" is a non-standard OTG. Seen as people really
didn't care about OTG, we (linux-usb folks) ended up naturally referring
to "non-standard OTG" as "dual-role". Just to avoid confusion.

> You are right, if the connector is type-c, it will be called as "type-c
> port" by people :)

oh no, that's not what I'm talking about. If you read Type-C and PD
specs, they define their own method for data role swapping. USB OTG
doesn't fit on top of a Type-C environment. It's not about what people
will call it, it's really that OTG can't work on top of type-c. For
starters, there's no ID pin ;-)

-- 
balbi


signature.asc
Description: PGP signature