Prev: tty: usb_serial_mos7720: Fix get_lsr_info
Next: [PATCH] block: fix bio_add_page for non trivial merge_bvec_fn case
From: David Brownell on 26 Jan 2010 07:20
On Friday 11 December 2009, Felipe Balbi wrote:
> The notifier will be used to communicate usb events
> to other drivers like the charger chip.

Good idea ... but not OTG-specific. It doesn't seem to me
that charging hookups belong in that header at all.

In fact, usb_gadget_vbus_draw() might better be implemented
as such a notifier call, removing that configuration mess
from the usb gadget drivers ("how can I know what charger
to use??"). That would be the most common situation: a
peripheral-only device.

And in fact, OTG can be treated as a trivial superset of
peripheral-only USB. (In terms of charger support, only!!)

I'd vote to convert all the USB-to-charger interfaces so
they use notifiers. After fixing the events ... see
comments below. :)


> This can be used as source of information to kick
> usb charger detection as described by the USB
> Battery Charging Specification 1.1 and/or to
> pass bMaxPower field of selected usb_configuration
> to charger chip in order to use that information
> as input current on the charging profile
> setup.
>
> Signed-off-by: Felipe Balbi <felipe.balbi(a)nokia.com>
> ---
> include/linux/usb/otg.h | 25 +++++++++++++++++++++++++
> 1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> index 52bb917..6c0b676 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -9,6 +9,8 @@
> #ifndef __LINUX_USB_OTG_H
> #define __LINUX_USB_OTG_H
>
> +#include <linux/notifier.h>
> +
> /* OTG defines lots of enumeration states before device reset */
> enum usb_otg_state {
> OTG_STATE_UNDEFINED = 0,
> @@ -33,6 +35,14 @@ enum usb_otg_state {
> OTG_STATE_A_VBUS_ERR,
> };
>
> +enum usb_xceiv_events {

Let's keep charger events separate from anything else,
like "enter host mode" or "enter peripheral mode" (or
even "disconnect"). The audiences for any other types
of event would be entirely different.

Right now there's a mess in terms of charger hookup,
so getting that straight is IMO a priority over any
other type of event. Using events will decouple a
bunch of drivers, and simplify driver configuration.


> + USB_EVENT_NONE, /* no events or cable disconnected */
> + USB_EVENT_VBUS, /* vbus valid event */
> + USB_EVENT_ID, /* id was grounded */
> + USB_EVENT_CHARGER, /* usb dedicated charger */
> + USB_EVENT_ENUMERATED, /* gadget driver enumerated */

Those seem like the wrong events. The right events for a charger
would be more along the lines of:

- For peripheral: "you may use N milliAmperes now".
- General: "Don't Charge" (a.k.a. "use 0 mA").

I don't see how "N" would be passed with those events ...

Haven't looked at the details of the charger spec, but
those two events are the *basics* from the USB 2.0 spec,
so "official" charger hardware wouldn't be less capable.

Thing like different levels of VBUS validity, ID grounding,
and so forth ... wouldn't be very relevant. An OTG driver
will do various things, internally, when ID grounds; but
anything else is a function of what role eventually gets
negotiated. And for the charger, they all add up to "Don't
Charge" (since ID grounded means A-role, sourcing power).

A host *might* want to be able to say things like "supply
up to N milliAmperes now", e.g. to let a regulator choose
the most efficient mode.


> +};
> +
> #define USB_OTG_PULLUP_ID (1 << 0)
> #define USB_OTG_PULLDOWN_DP (1 << 1)
> #define USB_OTG_PULLDOWN_DM (1 << 2)
> @@ -70,6 +80,9 @@ struct otg_transceiver {
> struct otg_io_access_ops *io_ops;
> void __iomem *io_priv;
>
> + /* for notification of usb_xceiv_events */
> + struct blocking_notifier_head notifier;

Why "blocking"? That seems kind of unnatural; for example,
the main users -- like usb_gadget_vbus_draw() -- would be
called in IRQ context (blocking not allowed).

> +
> /* to pass extra port status to the root hub */
> u16 port_status;
> u16 port_change;
> @@ -203,6 +216,18 @@ otg_start_srp(struct otg_transceiver *otg)
> return otg->start_srp(otg);
> }
>
> +/* notifiers */
> +static inline int
> +otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&otg->notifier, nb);
> +}
> +
> +static inline void
> +otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
> +{
> + blocking_notifier_chain_unregister(&otg->notifier, nb);
> +}
>
> /* for OTG controller drivers (and maybe other stuff) */
> extern int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 26 Jan 2010 08:20
On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:

> I'd vote to convert all the USB-to-charger interfaces so
> they use notifiers. After fixing the events ... see
> comments below. :)

Yes please - it's not just chargers either, this can also be used by
PMICs which do power path management that includes USB.

> Those seem like the wrong events. The right events for a charger
> would be more along the lines of:

> - For peripheral: "you may use N milliAmperes now".
> - General: "Don't Charge" (a.k.a. "use 0 mA").

> I don't see how "N" would be passed with those events ...

These are good for the peripheral side. You do get to pass a void *
along with the notifier value, that could be used to pass data like the
current limit.

> A host *might* want to be able to say things like "supply
> up to N milliAmperes now", e.g. to let a regulator choose
> the most efficient mode.

Yes, they definitely want this - not just for efficiency but also to
allow current limiting to protect the system from excessive current
drain.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on 26 Jan 2010 08:40
On Tuesday 26 January 2010, Mark Brown wrote:
> On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:
>
> > I'd vote to convert all the USB-to-charger interfaces so
> > they use notifiers. After fixing the events ... see
> > comments below. :)
>
> Yes please - it's not just chargers either, this can also be used by
> PMICs which do power path management that includes USB.

Color me confused ... what do you mean by "power path"?

Do you mean something like "the board as a whole can take N mA of
current from USB", rather than specifically addressing a charger?

It's not uncommon to do things like use VBUS current to power the
USB circuitry, too. That can leave less for other purposes. All
of that being rather board-specific.


> > Those seem like the wrong events. The right events for a charger
> > would be more along the lines of:
>
> > - For peripheral: "you may use N milliAmperes now".
> > - General: "Don't Charge" (a.k.a. "use 0 mA").
>
> > I don't see how "N" would be passed with those events ...
>
> These are good for the peripheral side. You do get to pass a void *
> along with the notifier value, that could be used to pass data like the
> current limit.

I don't think I saw that being done ... either in code, comments,
or documentation. Passing N is fundamental.


> > A host *might* want to be able to say things like "supply
> > up to N milliAmperes now", e.g. to let a regulator choose
> > the most efficient mode.
>
> Yes, they definitely want this - not just for efficiency but also to
> allow current limiting to protect the system from excessive current
> drain.

There are load bursting issues too. All part of the USB spec;
a load that's OK for 1 millisecond might not be OK for 1 second.
ISTR the "supply N mA" refers to an average. And there are some
limits to the capacitance that can practically be stuck on VBUS
output lines (which includes the cable). Solvable problems, but
not always pretty if software has to think it through.

Thing is, supplying current is a bit more involved. If the
board can't supply 300 mA, the USB configuration selection
mechanism has to know that, so it never selects peripheral
configurations which require that much current.

Or maybe these two ports can serve 500 mA, but not those two;
or their total can't exceed N (function of componentry and power
budgeting).

Ergo my desire to start with a straightforward problem whose
solution has real value (how much VBUS current may be consumed?),
and leave some of those other messes for later!

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Felipe Balbi on 26 Jan 2010 09:20
On Tue, Jan 26, 2010 at 02:35:21PM +0100, ext David Brownell wrote:
>On Tuesday 26 January 2010, Mark Brown wrote:
>> On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:
>>
>> > I'd vote to convert all the USB-to-charger interfaces so
>> > they use notifiers. After fixing the events ... see
>> > comments below. :)
>>
>> Yes please - it's not just chargers either, this can also be used by
>> PMICs which do power path management that includes USB.
>
>Color me confused ... what do you mean by "power path"?
>
>Do you mean something like "the board as a whole can take N mA of
>current from USB", rather than specifically addressing a charger?
>
>It's not uncommon to do things like use VBUS current to power the
>USB circuitry, too. That can leave less for other purposes. All
>of that being rather board-specific.
>
>
>> > Those seem like the wrong events. The right events for a charger
>> > would be more along the lines of:
>>
>> > - For peripheral: "you may use N milliAmperes now".
>> > - General: "Don't Charge" (a.k.a. "use 0 mA").
>>
>> > I don't see how "N" would be passed with those events ...
>>
>> These are good for the peripheral side. You do get to pass a void *
>> along with the notifier value, that could be used to pass data like the
>> current limit.
>
>I don't think I saw that being done ... either in code, comments,
>or documentation. Passing N is fundamental.

yeah, my bad. I should have said that, but it's more related to the
implementation of the notifier_block.

>> > A host *might* want to be able to say things like "supply
>> > up to N milliAmperes now", e.g. to let a regulator choose
>> > the most efficient mode.
>>
>> Yes, they definitely want this - not just for efficiency but also to
>> allow current limiting to protect the system from excessive current
>> drain.
>
>There are load bursting issues too. All part of the USB spec;
>a load that's OK for 1 millisecond might not be OK for 1 second.

if you get a SetConfiguration(config), then you can use that load for as
long as needed, the limitation is when not enumerated, afaict.

>ISTR the "supply N mA" refers to an average. And there are some
>limits to the capacitance that can practically be stuck on VBUS
>output lines (which includes the cable). Solvable problems, but
>not always pretty if software has to think it through.
>
>Thing is, supplying current is a bit more involved. If the
>board can't supply 300 mA, the USB configuration selection
>mechanism has to know that, so it never selects peripheral
>configurations which require that much current.

but that's done already by the usb core, no ? It rules out configuration
based on the hub->power_budget (can't remember if the field is that
exact name).

--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Felipe Balbi on 26 Jan 2010 09:20
Hi,

On Tue, Jan 26, 2010 at 12:16:20PM +0100, ext David Brownell wrote:
>On Friday 11 December 2009, Felipe Balbi wrote:
>> The notifier will be used to communicate usb events
>> to other drivers like the charger chip.
>
>Good idea ... but not OTG-specific. It doesn't seem to me

thanks

>that charging hookups belong in that header at all.

I noticed that when started actually using it :-)

>In fact, usb_gadget_vbus_draw() might better be implemented
>as such a notifier call, removing that configuration mess
>from the usb gadget drivers ("how can I know what charger
>to use??"). That would be the most common situation: a
>peripheral-only device.
>
>And in fact, OTG can be treated as a trivial superset of
>peripheral-only USB. (In terms of charger support, only!!)
>
>I'd vote to convert all the USB-to-charger interfaces so
>they use notifiers. After fixing the events ... see
>comments below. :)

makes sense

>> @@ -9,6 +9,8 @@
>> #ifndef __LINUX_USB_OTG_H
>> #define __LINUX_USB_OTG_H
>>
>> +#include <linux/notifier.h>
>> +
>> /* OTG defines lots of enumeration states before device reset */
>> enum usb_otg_state {
>> OTG_STATE_UNDEFINED = 0,
>> @@ -33,6 +35,14 @@ enum usb_otg_state {
>> OTG_STATE_A_VBUS_ERR,
>> };
>>
>> +enum usb_xceiv_events {
>
>Let's keep charger events separate from anything else,
>like "enter host mode" or "enter peripheral mode" (or
>even "disconnect"). The audiences for any other types
>of event would be entirely different.

the idea was to notify USB events to interested drivers, not only "usb
charger events".

>Right now there's a mess in terms of charger hookup,
>so getting that straight is IMO a priority over any
>other type of event. Using events will decouple a
>bunch of drivers, and simplify driver configuration.

well, if you consider that this transceiver isn't really otg specific,
then this is already wrong.

>> + USB_EVENT_NONE, /* no events or cable disconnected */
>> + USB_EVENT_VBUS, /* vbus valid event */
>> + USB_EVENT_ID, /* id was grounded */
>> + USB_EVENT_CHARGER, /* usb dedicated charger */
>> + USB_EVENT_ENUMERATED, /* gadget driver enumerated */
>
>Those seem like the wrong events. The right events for a charger
>would be more along the lines of:
>
> - For peripheral: "you may use N milliAmperes now".
> - General: "Don't Charge" (a.k.a. "use 0 mA").

I have to disagree, which information would you used to kick the usb
dedicated charger detection other than VBUS irq from transceiver ?

So we need at least that, and also need to notify when the charger
detection is finished, so we can enable data pullups on the link.
Remember we might be connected to a charging downstream port.

>I don't see how "N" would be passed with those events ...

there's a void * we can use to pass bMaxPower field of the selected
configuration.

>Haven't looked at the details of the charger spec, but
>those two events are the *basics* from the USB 2.0 spec,
>so "official" charger hardware wouldn't be less capable.

I believe the dedicated charger is also "basic".

>Thing like different levels of VBUS validity, ID grounding,
>and so forth ... wouldn't be very relevant. An OTG driver
>will do various things, internally, when ID grounds; but
>anything else is a function of what role eventually gets
>negotiated. And for the charger, they all add up to "Don't
>Charge" (since ID grounded means A-role, sourcing power).

ID grounding event is necessary if you have an external charge pump.
At least the boards I've been working use an external chip as the USB
Charger and Charge pump, iow, the transceiver doesn't source VBUS on ID
ground, but the charger chip is put into "boost" mode for that role.

>> #define USB_OTG_PULLUP_ID (1 << 0)
>> #define USB_OTG_PULLDOWN_DP (1 << 1)
>> #define USB_OTG_PULLDOWN_DM (1 << 2)
>> @@ -70,6 +80,9 @@ struct otg_transceiver {
>> struct otg_io_access_ops *io_ops;
>> void __iomem *io_priv;
>>
>> + /* for notification of usb_xceiv_events */
>> + struct blocking_notifier_head notifier;
>
>Why "blocking"? That seems kind of unnatural; for example,
>the main users -- like usb_gadget_vbus_draw() -- would be
>called in IRQ context (blocking not allowed).

what about irqs running in thread, wouldn't we "BUG sleeping in irq
context" ?

--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4
Prev: tty: usb_serial_mos7720: Fix get_lsr_info
Next: [PATCH] block: fix bio_add_page for non trivial merge_bvec_fn case