| Message ID | e58e4f26028c37877d02907831cf4ae411eaf984.1443057231.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote: > The usb charger framework is based on usb gadget. The usb charger > need to be notified the state changing of usb gadget to confirm the > usb charger state. > > Thus this patch adds a notifier mechanism for usb gadget to report a > event to usb charger when the usb gadget state is changed. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/usb/gadget.h | 18 ++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c > index f660afb..4238fc3 100644 > --- a/drivers/usb/gadget/udc/udc-core.c > +++ b/drivers/usb/gadget/udc/udc-core.c > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep, > } > EXPORT_SYMBOL_GPL(usb_gadget_giveback_request); > > +int usb_gadget_register_notify(struct usb_gadget *gadget, > + struct notifier_block *nb) > +{ > + int ret; > + > + mutex_lock(&gadget->lock); > + ret = raw_notifier_chain_register(&gadget->nh, nb); > + mutex_unlock(&gadget->lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify); > + > +int usb_gadget_unregister_notify(struct usb_gadget *gadget, > + struct notifier_block *nb) > +{ > + int ret; > + > + mutex_lock(&gadget->lock); > + ret = raw_notifier_chain_unregister(&gadget->nh, nb); Greg, this is the kind of thing I wanted to avoid adding more of. I was wondering if you would accept subsystems using kdbus for this sort of notification. I'm okay waiting for kdbus for another couple merge windows (if we have to) before that's merged, but if we take this raw notifier approach now, we will end up having to support it forever. Also, because soon enough we will have to support USB Power Delivery with Type C connector, this is bound to change in the coming months. Frankly, I wanted all of this to be decided in userland with the kernel just providing notification and basic safety checks (we don't want to allow a bogus userspace daemon frying anybody's devices). How would you feel about that ?
On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > Frankly, I wanted all of this to be decided in userland with the > kernel just providing notification and basic safety checks (we don't > want to allow a bogus userspace daemon frying anybody's devices). What's the advantage of pushing this to userspace? By the time we provide enough discoverability to enable userspace to configure itself it seems like we'd have enough information to do the job anyway.
Hi, On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > Frankly, I wanted all of this to be decided in userland with the > > kernel just providing notification and basic safety checks (we don't > > want to allow a bogus userspace daemon frying anybody's devices). > > What's the advantage of pushing this to userspace? By the time we > provide enough discoverability to enable userspace to configure itself > it seems like we'd have enough information to do the job anyway. you're going to be dealing with a setup where each vendor does one thing differently. Some will have it all in the SoC part of a single IP (dwc3 can be configured that way), some will push it out to companion IC, some might even use some mostly discrete setup and so on... We're gonna be dealing with a decision that involves information from multiple subsystems (USB, regulator, hwmon, power supply to name a few). We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's just plain difficult. To make matters worse, N900 had two USB PHYs, one for actual runtime use and another just for detecting the charger type on the other end. On top of all that, we still need to figure out what to do when a particular configuration gets chosen, and what to do when the bus goes into the different suspend levels. It's going to be a lot of policy getting added to the kernel. On top of all that, when Type C and power delivery is finally a thing, there will an entire new stack for USB C commands (using the Type C CC pins or modulated on VBUS for legacy connectors) which we will have to add to the kernel. And different devices will support different charging profiles (there are at least 6 of those, IIRC). With all these different things going on, it's best to do what e.g. NFC folks did. Just a small set of hooks in the kernel, but actual implementation done by a userspace daemon. This makes it even easier for middleware development since we can provide a higher level API for middleware to talk to the charging daemon. Another benefit is that this charging daemon can also give hints to power management policy that e.g. we're charging at 10W or 100W and we can e.g. switch to performance governor safely even though battery is rather low. Anyway, there's really a lot that needs to happen and shuving it all in the kernel is, IMHO, the wrong decision. I would be much happier with minimal safety requirements in the kernel and let a userspace daemon (which we should certainly provide a reference implementation) figure out what to do. This might be better for things like Chromebooks and Android phones too which might make completely different decisions given a certain charging profile.
On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote: > Hi, > > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > > > Frankly, I wanted all of this to be decided in userland with the > > > kernel just providing notification and basic safety checks (we don't > > > want to allow a bogus userspace daemon frying anybody's devices). > > > > What's the advantage of pushing this to userspace? By the time we > > provide enough discoverability to enable userspace to configure itself > > it seems like we'd have enough information to do the job anyway. > > you're going to be dealing with a setup where each vendor does one thing > differently. Some will have it all in the SoC part of a single IP (dwc3 can be > configured that way), some will push it out to companion IC, some might even use oh, and as for dwc3 itself: it *can* be configured that way, but all those charging blocks are optional :-) So you will even have setups where the very same IP works differently because SoC vendor A configured it differently from SoC vendor B.
On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote: > > The usb charger framework is based on usb gadget. The usb charger > > need to be notified the state changing of usb gadget to confirm the > > usb charger state. > > > > Thus this patch adds a notifier mechanism for usb gadget to report a > > event to usb charger when the usb gadget state is changed. > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > --- > > drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++ > > include/linux/usb/gadget.h | 18 ++++++++++++++++++ > > 2 files changed, 50 insertions(+) > > > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c > > index f660afb..4238fc3 100644 > > --- a/drivers/usb/gadget/udc/udc-core.c > > +++ b/drivers/usb/gadget/udc/udc-core.c > > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep, > > } > > EXPORT_SYMBOL_GPL(usb_gadget_giveback_request); > > > > +int usb_gadget_register_notify(struct usb_gadget *gadget, > > + struct notifier_block *nb) > > +{ > > + int ret; > > + > > + mutex_lock(&gadget->lock); > > + ret = raw_notifier_chain_register(&gadget->nh, nb); > > + mutex_unlock(&gadget->lock); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify); > > + > > +int usb_gadget_unregister_notify(struct usb_gadget *gadget, > > + struct notifier_block *nb) > > +{ > > + int ret; > > + > > + mutex_lock(&gadget->lock); > > + ret = raw_notifier_chain_unregister(&gadget->nh, nb); > > Greg, this is the kind of thing I wanted to avoid adding more of. > > I was wondering if you would accept subsystems using kdbus for > this sort of notification. I'm okay waiting for kdbus for another > couple merge windows (if we have to) before that's merged, but > if we take this raw notifier approach now, we will end up having > to support it forever. kdbus is userspace <-> userspace messages, it doesn't do kernel <-> userspace messages, sorry. > Also, because soon enough we will have to support USB Power Delivery > with Type C connector, this is bound to change in the coming months. > > Frankly, I wanted all of this to be decided in userland with the > kernel just providing notification and basic safety checks (we don't > want to allow a bogus userspace daemon frying anybody's devices). > > How would you feel about that ? I agree I don't like new notifier chains being added, but as this is all in-kernel, we can change the api in the future and there would not be a problem, right? And yeah, I'm worried about the USB Power delivery patches, I haven't seen them yet, but I hear about different groups doing different things here, and that worries me. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote: > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > Frankly, I wanted all of this to be decided in userland with the > > > kernel just providing notification and basic safety checks (we don't > > > want to allow a bogus userspace daemon frying anybody's devices). > > What's the advantage of pushing this to userspace? By the time we > > provide enough discoverability to enable userspace to configure itself > > it seems like we'd have enough information to do the job anyway. > you're going to be dealing with a setup where each vendor does one thing > differently. Some will have it all in the SoC part of a single IP (dwc3 can be > configured that way), some will push it out to companion IC, some might even use > some mostly discrete setup and so on... Right, and that was exactly what this was supposed to be all about when I was discussing this originally with Baolin (who's on holiday until sometime next week BTW, hence me answering). > We're gonna be dealing with a decision that involves information from multiple > subsystems (USB, regulator, hwmon, power supply to name a few). Sure, that was part of the idea here - provide a central point representing the logical port where we can aggregate all the information we get in and distribute it to consumers. > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's > just plain difficult. To make matters worse, N900 had two USB PHYs, one for > actual runtime use and another just for detecting the charger type on the other > end. Can you elaborate on what the difficulties are and how punting to userspace helps? If anything I'd expect punting to userspace to make things more difficult, if nothing else it means we need to get whatever userspace component deployed in all relevant userspace stacks with appropriate configuration in order to do things. We do punt a lot of configuration to userspace for audio because there are substantial device specific policy elements in the configuration and it's a far from painless experience getting the code and configuration deployed where people need it, definitely a not great for users. > On top of all that, we still need to figure out what to do when a particular > configuration gets chosen, and what to do when the bus goes into the different > suspend levels. > It's going to be a lot of policy getting added to the kernel. On top of all > that, when Type C and power delivery is finally a thing, there will an entire > new stack for USB C commands (using the Type C CC pins or modulated on VBUS for > legacy connectors) which we will have to add to the kernel. And different > devices will support different charging profiles (there are at least 6 of those, > IIRC). Yes, USB C was part of the thinking with starting this work - it's going to make ensuring that the system is paying attention to limits much more important. I think there's two things here. One is working out how the system is glued together and the other thing is working out what to do with that system. I think that where we can do it the bit where work out how the various components are connected and aggregate their functionality together is definitely a kernel job. It means that userspace doesn't need to worry about implementation details of the particular system and instead gets a consistent, standard view of the device (in this case a USB port and how much power we can draw from it). For the policy stuff we do want to have userspace be able to control things but we need to think about how to do that - for example does the entire flow need to be in userspace, or can we just expose settings which userspace can manage? > With all these different things going on, it's best to do what e.g. NFC folks > did. Just a small set of hooks in the kernel, but actual implementation done by > a userspace daemon. This makes it even easier for middleware development since > we can provide a higher level API for middleware to talk to the charging daemon. I'm not sure that the NFC model is working well for everyone - it's OK if you happen to be running Android but if you aren't you're often left sitting there working out what to do with an Android HAL. We can do the middleware thing without going quite that far, we do already have power management daemons in userspace even on desktop (GNOME has upowerd for example). > Another benefit is that this charging daemon can also give hints to power > management policy that e.g. we're charging at 10W or 100W and we can e.g. switch > to performance governor safely even though battery is rather low. We definitely want userspace to be able to see the current state, even if it just passes it straight on to the user it's a common part of UIs on both desktop and mobile operating systems. > Anyway, there's really a lot that needs to happen and shuving it all in the > kernel is, IMHO, the wrong decision. I would be much happier with minimal safety > requirements in the kernel and let a userspace daemon (which we should certainly > provide a reference implementation) figure out what to do. This might be better > for things like Chromebooks and Android phones too which might make completely > different decisions given a certain charging profile. Saying we don't want to have absolutely everything in kernel space doesn't mean we should have nothing at all there, doing that seems like going too far in the other direction.
Hi, On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote: > > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > > > Frankly, I wanted all of this to be decided in userland with the > > > > kernel just providing notification and basic safety checks (we don't > > > > want to allow a bogus userspace daemon frying anybody's devices). > > > > What's the advantage of pushing this to userspace? By the time we > > > provide enough discoverability to enable userspace to configure itself > > > it seems like we'd have enough information to do the job anyway. > > > > you're going to be dealing with a setup where each vendor does one thing > > differently. Some will have it all in the SoC part of a single IP (dwc3 can be > > configured that way), some will push it out to companion IC, some might even use > > some mostly discrete setup and so on... > > Right, and that was exactly what this was supposed to be all about when > I was discussing this originally with Baolin (who's on holiday until > sometime next week BTW, hence me answering). > > > We're gonna be dealing with a decision that involves information from multiple > > subsystems (USB, regulator, hwmon, power supply to name a few). > > Sure, that was part of the idea here - provide a central point > representing the logical port where we can aggregate all the information > we get in and distribute it to consumers. > > > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's > > just plain difficult. To make matters worse, N900 had two USB PHYs, one for > > actual runtime use and another just for detecting the charger type on the other > > end. > > Can you elaborate on what the difficulties are and how punting to > userspace helps? If anything I'd expect punting to userspace to make the difficulty was mainly making sure all involved parties talk the same language. I mean, you plug cable and detect charger (this is usually done by the PMIC or by a BQ-type device - probably done at drivers/power_supply). First difficulty comes right here. Power_supply notifies that we're attached to a charger (sometimes it can't differentiate a host/hub charger from a wall charger), a few ms later you get a notification from the gadget that it got enumerated with a 500mA configuration. Depending on the order of things, your load will be configured either to 2A (maximum host/hub charger current) or 500mA. Yes, this can be mitigated :-) Focussing on this alone, you can have as much as 4 different subsystems involved, all throwing raw_notifiers at each other. Now each of these subsystems need to maintain their own state machine about what notification we have received and what are the valid next states. I would rather have userspace be the single place getting notifications because then we solve these details at a single location. > Things more difficult, if nothing else it means we need to get whatever > userspace component deployed in all relevant userspace stacks with > appropriate configuration in order to do things. but that will be a thing for the kernel too. We will have to deploy this new kernel component in all relevant stacks with appropriate configuration in order to do things :-) No changes there. > We do punt a lot of configuration to userspace for audio because there > are substantial device specific policy elements in the configuration and > it's a far from painless experience getting the code and configuration > deployed where people need it, definitely a not great for users. it's the same for this situation. We will have a ton of device specific configuration, specially with power delivery class, billboard class, the alternate modes and so on. If we already do this part in userland, adding all those extras will be, IMO, far simpler. > > On top of all that, we still need to figure out what to do when a particular > > configuration gets chosen, and what to do when the bus goes into the different > > suspend levels. > > > It's going to be a lot of policy getting added to the kernel. On top of all > > that, when Type C and power delivery is finally a thing, there will an entire > > new stack for USB C commands (using the Type C CC pins or modulated on VBUS for > > legacy connectors) which we will have to add to the kernel. And different > > devices will support different charging profiles (there are at least 6 of those, > > IIRC). > > Yes, USB C was part of the thinking with starting this work - it's going > to make ensuring that the system is paying attention to limits much more > important. I think there's two things here. One is working out how the > system is glued together and the other thing is working out what to do > with that system. right, and IMO, what to do should be a decision made outside of the kernel as long as kernel provides safety. > I think that where we can do it the bit where work out how the various > components are connected and aggregate their functionality together is > definitely a kernel job. It means that userspace doesn't need to worry > about implementation details of the particular system and instead gets a > consistent, standard view of the device (in this case a USB port and how > much power we can draw from it). Actually, I was thinking about it in a more granular fashion. Kernel exposes a charger/power_supply, a few regulators, a UDC view and this single userspace daemon figures out how to tie those together. The view here isn't really a USB port, it's just a source of power. But how much power we can draw from it depends on, possibly, a ton of different chips and different notifications. > For the policy stuff we do want to have userspace be able to control > things but we need to think about how to do that - for example does the > entire flow need to be in userspace, or can we just expose settings > which userspace can manage? considering the amount of different designs that are already out there and all the extras that are going to show up due to type-c, I think we'd be better off exposing as much to userspace as possible. > > With all these different things going on, it's best to do what e.g. NFC folks > > did. Just a small set of hooks in the kernel, but actual implementation done by > > a userspace daemon. This makes it even easier for middleware development since > > we can provide a higher level API for middleware to talk to the charging daemon. > > I'm not sure that the NFC model is working well for everyone - it's OK > if you happen to be running Android but if you aren't you're often left > sitting there working out what to do with an Android HAL. We can do the NFC works pretty well for neard, afaict. And without android. > middleware thing without going quite that far, we do already have power > management daemons in userspace even on desktop (GNOME has upowerd for > example). right > > Another benefit is that this charging daemon can also give hints to power > > management policy that e.g. we're charging at 10W or 100W and we can e.g. switch > > to performance governor safely even though battery is rather low. > > We definitely want userspace to be able to see the current state, even > if it just passes it straight on to the user it's a common part of UIs > on both desktop and mobile operating systems. > > > Anyway, there's really a lot that needs to happen and shuving it all in the > > kernel is, IMHO, the wrong decision. I would be much happier with minimal safety > > requirements in the kernel and let a userspace daemon (which we should certainly > > provide a reference implementation) figure out what to do. This might be better > > for things like Chromebooks and Android phones too which might make completely > > different decisions given a certain charging profile. > > Saying we don't want to have absolutely everything in kernel space > doesn't mean we should have nothing at all there, doing that seems like > going too far in the other direction. we _have_ to have something in the kernel :-) I'm arguing that we might not want as much as you think we do :-) The real difficulty here is coming up with an interface which we're agreeing to supporting for the next 30 years. Whatever that is, as soon as it gets exposed to userland, we will have to support it. And this another reason I think a more granular approach is better, as it allows us to easily add more pieces as they come along.
Hi, On Fri, Oct 02, 2015 at 07:41:07AM +0200, Greg KH wrote: > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote: > > > The usb charger framework is based on usb gadget. The usb charger > > > need to be notified the state changing of usb gadget to confirm the > > > usb charger state. > > > > > > Thus this patch adds a notifier mechanism for usb gadget to report a > > > event to usb charger when the usb gadget state is changed. > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > --- > > > drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++ > > > include/linux/usb/gadget.h | 18 ++++++++++++++++++ > > > 2 files changed, 50 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c > > > index f660afb..4238fc3 100644 > > > --- a/drivers/usb/gadget/udc/udc-core.c > > > +++ b/drivers/usb/gadget/udc/udc-core.c > > > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep, > > > } > > > EXPORT_SYMBOL_GPL(usb_gadget_giveback_request); > > > > > > +int usb_gadget_register_notify(struct usb_gadget *gadget, > > > + struct notifier_block *nb) > > > +{ > > > + int ret; > > > + > > > + mutex_lock(&gadget->lock); > > > + ret = raw_notifier_chain_register(&gadget->nh, nb); > > > + mutex_unlock(&gadget->lock); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify); > > > + > > > +int usb_gadget_unregister_notify(struct usb_gadget *gadget, > > > + struct notifier_block *nb) > > > +{ > > > + int ret; > > > + > > > + mutex_lock(&gadget->lock); > > > + ret = raw_notifier_chain_unregister(&gadget->nh, nb); > > > > Greg, this is the kind of thing I wanted to avoid adding more of. > > > > I was wondering if you would accept subsystems using kdbus for > > this sort of notification. I'm okay waiting for kdbus for another > > couple merge windows (if we have to) before that's merged, but > > if we take this raw notifier approach now, we will end up having > > to support it forever. > > kdbus is userspace <-> userspace messages, it doesn't do kernel <-> > userspace messages, sorry. oh well, tough luck :-) having a more well-constructed notification method to userspace would be a little better, but I guess we will have to resort to sysfs_notify() or something like that. > > Also, because soon enough we will have to support USB Power Delivery > > with Type C connector, this is bound to change in the coming months. > > > > Frankly, I wanted all of this to be decided in userland with the > > kernel just providing notification and basic safety checks (we don't > > want to allow a bogus userspace daemon frying anybody's devices). > > > > How would you feel about that ? > > I agree I don't like new notifier chains being added, but as this is all > in-kernel, we can change the api in the future and there would not be a > problem, right? not necessarily. This will, eventually, get exposed to userspace. At minimum for adding eye candy to the UI. Chaging battery icon or whatever. The danger here is that when that something gets exposed, we will have to support it for the next 30 years :-) > And yeah, I'm worried about the USB Power delivery patches, I haven't > seen them yet, but I hear about different groups doing different things > here, and that worries me. my worries exactly :-) This is why I'm looking for a flexible enough approach which puts as little into the kernel as necessary.
On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > > Can you elaborate on what the difficulties are and how punting to > > userspace helps? If anything I'd expect punting to userspace to make > the difficulty was mainly making sure all involved parties talk the same > language. I mean, you plug cable and detect charger (this is usually done by the > PMIC or by a BQ-type device - probably done at drivers/power_supply). > First difficulty comes right here. Power_supply notifies that we're attached to > a charger (sometimes it can't differentiate a host/hub charger from a wall > charger), a few ms later you get a notification from the gadget that it got > enumerated with a 500mA configuration. Depending on the order of things, your > load will be configured either to 2A (maximum host/hub charger current) or > 500mA. Yes, this can be mitigated :-) > Focussing on this alone, you can have as much as 4 different subsystems > involved, all throwing raw_notifiers at each other. Now each of these subsystems > need to maintain their own state machine about what notification we have > received and what are the valid next states. OK, so part of the goal here was to outsource all the state machines to some generic code - what you're describing here is definitely a problem and the idea was to provide a central place that has the logic and makes the decisions about what we should be doing. If we don't have that it's going to get messy. > I would rather have userspace be the single place getting notifications because > then we solve these details at a single location. No argument on the place, making that place be userspace I'm less sold on. > > Things more difficult, if nothing else it means we need to get whatever > > userspace component deployed in all relevant userspace stacks with > > appropriate configuration in order to do things. > but that will be a thing for the kernel too. We will have to deploy this new > kernel component in all relevant stacks with appropriate configuration in order > to do things :-) No changes there. Sure, but it's one project which is developed entirely in the open - that's a lot more manageable. > > We do punt a lot of configuration to userspace for audio because there > > are substantial device specific policy elements in the configuration and > > it's a far from painless experience getting the code and configuration > > deployed where people need it, definitely a not great for users. > it's the same for this situation. We will have a ton of device specific > configuration, specially with power delivery class, billboard class, the > alternate modes and so on. If we already do this part in userland, adding all > those extras will be, IMO, far simpler. Can you elaborate a bit? As far as I can tell all those are still in the generic USB space rather than the sort of device specifics I'm thinking of. The things we've got with audio are a combination of tuning to taste and (even more importantly) very different ideas about what you want to do with an audio subsystem that influence how you set things up in a given use case. > > I think that where we can do it the bit where work out how the various > > components are connected and aggregate their functionality together is > > definitely a kernel job. It means that userspace doesn't need to worry > > about implementation details of the particular system and instead gets a > > consistent, standard view of the device (in this case a USB port and how > > much power we can draw from it). > Actually, I was thinking about it in a more granular fashion. Kernel exposes a > charger/power_supply, a few regulators, a UDC view and this single userspace > daemon figures out how to tie those together. That sounds worrying - it means that new hardware support needs supporting in every userspace (it seems inevitable that there will be at least some reimplementations because that's fun) which gets old really fast, and every userspace needs to understand every topology. > The view here isn't really a USB port, it's just a source of power. But how much > power we can draw from it depends on, possibly, a ton of different chips and > different notifications. Right, yes - it's the power input/output associated with the USB port. The USB port is just the physical thing users can see so it's a good way to label it. That could mean that we ought to move the aggregation bit into the power supply code of course, but then a lot of the decisions are going to be specific to USB. > > For the policy stuff we do want to have userspace be able to control > > things but we need to think about how to do that - for example does the > > entire flow need to be in userspace, or can we just expose settings > > which userspace can manage? > considering the amount of different designs that are already out there and all > the extras that are going to show up due to type-c, I think we'd be better off > exposing as much to userspace as possible. How different are the end goals of these designs really going to be though - that's more of the question for me? Part of what the kernel is doing is hiding implementation details from userspace. I'd expect userspace to be interested in things like the maximum current allowed in or out in a given mode rather than the details of how that is accomplished. > > I'm not sure that the NFC model is working well for everyone - it's OK > > if you happen to be running Android but if you aren't you're often left > > sitting there working out what to do with an Android HAL. We can do the > NFC works pretty well for neard, afaict. And without android. Ah, that looks better than it was now - it looks like they're providing a reasonable hardware abstraction for messaging. Still, there's other examples of this model that are less successful where the kernel interface isn't generic. > The real difficulty here is coming up with an interface which we're agreeing to > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > to userland, we will have to support it. To me that sounds like an argument for hiding things :) > And this another reason I think a more granular approach is better, as it allows > us to easily add more pieces as they come along. OTOH it's more work for the system as a whole.
On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote: > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > > > > Can you elaborate on what the difficulties are and how punting to > > > userspace helps? If anything I'd expect punting to userspace to make > > > the difficulty was mainly making sure all involved parties talk the same > > language. I mean, you plug cable and detect charger (this is usually done by the > > PMIC or by a BQ-type device - probably done at drivers/power_supply). > > > First difficulty comes right here. Power_supply notifies that we're attached to > > a charger (sometimes it can't differentiate a host/hub charger from a wall > > charger), a few ms later you get a notification from the gadget that it got > > enumerated with a 500mA configuration. Depending on the order of things, your > > load will be configured either to 2A (maximum host/hub charger current) or > > 500mA. Yes, this can be mitigated :-) > > > Focussing on this alone, you can have as much as 4 different subsystems > > involved, all throwing raw_notifiers at each other. Now each of these subsystems > > need to maintain their own state machine about what notification we have > > received and what are the valid next states. > > OK, so part of the goal here was to outsource all the state machines to > some generic code - what you're describing here is definitely a problem > and the idea was to provide a central place that has the logic and makes > the decisions about what we should be doing. If we don't have that it's > going to get messy. > > > I would rather have userspace be the single place getting notifications because > > then we solve these details at a single location. > > No argument on the place, making that place be userspace I'm less sold > on. fair enough. This would be an entire new entity, though, and users (chargers, battery chips, USB controllers, USB PHYs, etc) should blindly notify this entity and no care about what happened before or what are possible next states. As long as we can guarantee that, then I'd be okay adding this to the kernel. > > > Things more difficult, if nothing else it means we need to get whatever > > > userspace component deployed in all relevant userspace stacks with > > > appropriate configuration in order to do things. > > > but that will be a thing for the kernel too. We will have to deploy this new > > kernel component in all relevant stacks with appropriate configuration in order > > to do things :-) No changes there. > > Sure, but it's one project which is developed entirely in the open - > that's a lot more manageable. We can have a fully open source userland daemon too. Much like systemd, bluez, neard, and many, many others. Why wouldn't that be a thing ? > > > We do punt a lot of configuration to userspace for audio because there > > > are substantial device specific policy elements in the configuration and > > > it's a far from painless experience getting the code and configuration > > > deployed where people need it, definitely a not great for users. > > > it's the same for this situation. We will have a ton of device specific > > configuration, specially with power delivery class, billboard class, the > > alternate modes and so on. If we already do this part in userland, adding all > > those extras will be, IMO, far simpler. > > Can you elaborate a bit? As far as I can tell all those are still in > the generic USB space rather than the sort of device specifics I'm > thinking of. let's consider the billboard class, for example. With the new Type-C connector, the extra CC pins get added and power delivery messaging goes on top of that. Billboard class gives us the opportunity to send a power delivery request to mux the data pins on the TypeC connector to something completely non-USB. So we could run NATIVE PCIe (single lane though) on top of this. Native DisplayPort, Native Thunderbolt (which the Fruit company announced will switch to type-C, so that will be a thing), native VGA... the list goes on and on. At that point, this is not entirely USB realm anymore :-) Similar applies to power delivery charging profile themselves. Not all profiles are mandatory. Some are optional and that will be completely device/board specific. How an OEM implements that in the PCB is also completely board-specific :-) Some might have a dumb IC hardcoding some messages, some might have something largely more flexible. > The things we've got with audio are a combination of tuning to taste and > (even more importantly) very different ideas about what you want to do > with an audio subsystem that influence how you set things up in a given > use case. the same thing will happen with Type-C and power delivery. There won't be tuning to taste, of course :-) But there will be "very different ideas what what you want to do with" your charging methodology. > > > I think that where we can do it the bit where work out how the various > > > components are connected and aggregate their functionality together is > > > definitely a kernel job. It means that userspace doesn't need to worry > > > about implementation details of the particular system and instead gets a > > > consistent, standard view of the device (in this case a USB port and how > > > much power we can draw from it). > > > Actually, I was thinking about it in a more granular fashion. Kernel exposes a > > charger/power_supply, a few regulators, a UDC view and this single userspace > > daemon figures out how to tie those together. > > That sounds worrying - it means that new hardware support needs > supporting in every userspace (it seems inevitable that there will be at > least some reimplementations because that's fun) which gets old really > fast, and every userspace needs to understand every topology. very true, but it's also true for a kernel solution. It seems like you want it in the kernel because of it being convenient :-) > > The view here isn't really a USB port, it's just a source of power. But how much > > power we can draw from it depends on, possibly, a ton of different chips and > > different notifications. > > Right, yes - it's the power input/output associated with the USB port. > The USB port is just the physical thing users can see so it's a good way > to label it. That could mean that we ought to move the aggregation bit > into the power supply code of course, but then a lot of the decisions > are going to be specific to USB. in a sense, yes. But they can happen at a layer which knows nothing (or very little) about USB. Here's an example: Not everybody follows USB Battery Charging class. Some chargers don't short D+/D- to signify they are a wall charger. Some will use different resistance values on the ID pin to tell you how much current you can draw from them. From a PMIC (or something else) point of view, all it needs is a tiny current source and a an ADC, then it reports the ADC value to the upper layer. This really doesn't even need to know that it's using an ID pin, or whatever. > > > For the policy stuff we do want to have userspace be able to control > > > things but we need to think about how to do that - for example does the > > > entire flow need to be in userspace, or can we just expose settings > > > which userspace can manage? > > > considering the amount of different designs that are already out there and all > > the extras that are going to show up due to type-c, I think we'd be better off > > exposing as much to userspace as possible. > > How different are the end goals of these designs really going to be > though - that's more of the question for me? Part of what the kernel is > doing is hiding implementation details from userspace. I'd expect > userspace to be interested in things like the maximum current allowed in > or out in a given mode rather than the details of how that is accomplished. okay, this is a fair point :-) I just don't see, as of now at least, how we can get to that in a way that's somewhat future-proof. It seems like we will add more and more notifications until we can't maintain this anymore. Say, we get a "certifiable" USB BC1.2 solution in the kernel. Current systems are happy, we drop yet another out-of-tree set of patches from AOSP, every holds hands and sings Kumbaya. In another year or so, power delivery will be *the* thing. Now all the work done assuming BC1.2 is gone, outdated. Adding power delivery brings an entire new set of requirements. A new protocol stack (yes! it runs on CC pins, remember ?), new messages, etc etc... With Type-C there's no port type anymore. What we used to refer as Host/Hub Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single type (let's call it type-c for short) which can provide profiles. These profiles are negotiated using that new stack I pointed out above, not by checking resistance on data lines or ID pin. If we're not careful, it will be really difficult to make power delivery fit into this. Add to that the fact that the same power delivery pipe (the CC pins) can be used to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan :-) In short, whatever we do, needs to consider coping with incoming requests from userspace at a minimum because userspace will need control over the port alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but it'll be a pain to make a flexible/generic solution :-) > > > I'm not sure that the NFC model is working well for everyone - it's OK > > > if you happen to be running Android but if you aren't you're often left > > > sitting there working out what to do with an Android HAL. We can do the > > > NFC works pretty well for neard, afaict. And without android. > > Ah, that looks better than it was now - it looks like they're providing > a reasonable hardware abstraction for messaging. Still, there's other > examples of this model that are less successful where the kernel > interface isn't generic. > > > The real difficulty here is coming up with an interface which we're agreeing to > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > > to userland, we will have to support it. > > To me that sounds like an argument for hiding things :) the problem with hiding, though, is that once something new comes about, it might not fit our current abstraction and it might be big PITA to support "old" and new systems. > > And this another reason I think a more granular approach is better, as it allows > > us to easily add more pieces as they come along. > > OTOH it's more work for the system as a whole. it's more work outside the kernel, yes. The total amount of work (kernel + userspace) will be the same, regardless if you hide it in the kernel or in userspace.
On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote: > On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote: > > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > > > > Things more difficult, if nothing else it means we need to get whatever > > > > userspace component deployed in all relevant userspace stacks with > > > > appropriate configuration in order to do things. > > > but that will be a thing for the kernel too. We will have to deploy this new > > > kernel component in all relevant stacks with appropriate configuration in order > > > to do things :-) No changes there. > > Sure, but it's one project which is developed entirely in the open - > > that's a lot more manageable. > We can have a fully open source userland daemon too. Much like systemd, bluez, > neard, and many, many others. Why wouldn't that be a thing ? The trouble is getting traction on adoption. Vendors have a habit of doing things like finding problems and rather than reporting them deciding that the open source solution isn't great and going and writing their own thing (especially when they're in stealth mode) rather than talking to anyone. Sometimes we manage to avoid that but it can be challenging, and often we only even hear about the new thing after it's shipping and there's a lot of inertia behind changing it. The kernel ABIs tend to be a lot sticker than userspace things here. > Similar applies to power delivery charging profile themselves. Not all profiles > are mandatory. Some are optional and that will be completely device/board > specific. How an OEM implements that in the PCB is also completely > board-specific :-) Some might have a dumb IC hardcoding some messages, some > might have something largely more flexible. Right, and what I was trying to say was that it seems like the kernel ought to be worrying about the board specific bits and userspace worrying about what to do with those bits. > > The things we've got with audio are a combination of tuning to taste and > > (even more importantly) very different ideas about what you want to do > > with an audio subsystem that influence how you set things up in a given > > use case. > the same thing will happen with Type-C and power delivery. There won't be tuning > to taste, of course :-) But there will be "very different ideas what what you > want to do with" your charging methodology. Are those very different things about the use cases ("don't bother with high speed data, get all the power you can" or whatever) or are they about the details of the board? > > > Actually, I was thinking about it in a more granular fashion. Kernel exposes a > > > charger/power_supply, a few regulators, a UDC view and this single userspace > > > daemon figures out how to tie those together. > > That sounds worrying - it means that new hardware support needs > > supporting in every userspace (it seems inevitable that there will be at > > least some reimplementations because that's fun) which gets old really > > fast, and every userspace needs to understand every topology. > very true, but it's also true for a kernel solution. > It seems like you want it in the kernel because of it being convenient :-) Yes, definitely - my experience both in terms of watching how people like handset vendors often work internally and in terms of getting things adopted is that it's a lot easier if you can have one component you need to update to handle new hardware rather than multiple ones that need to be upgraded for things that are purely board differences. > > > The view here isn't really a USB port, it's just a source of power. But how much > > > power we can draw from it depends on, possibly, a ton of different chips and > > > different notifications. > > Right, yes - it's the power input/output associated with the USB port. > > The USB port is just the physical thing users can see so it's a good way > > to label it. That could mean that we ought to move the aggregation bit > > into the power supply code of course, but then a lot of the decisions > > are going to be specific to USB. > in a sense, yes. But they can happen at a layer which knows nothing (or very > little) about USB. Here's an example: > Not everybody follows USB Battery Charging class. Some chargers don't short > D+/D- to signify they are a wall charger. Some will use different resistance > values on the ID pin to tell you how much current you can draw from them. > From a PMIC (or something else) point of view, all it needs is a tiny current > source and a an ADC, then it reports the ADC value to the upper layer. This > really doesn't even need to know that it's using an ID pin, or whatever. This doesn't seem much different to things like the various GPIO to subsystem X drivers we've got - we already have some for IIO type things IIRC. > > How different are the end goals of these designs really going to be > > though - that's more of the question for me? Part of what the kernel is > > doing is hiding implementation details from userspace. I'd expect > > userspace to be interested in things like the maximum current allowed in > > or out in a given mode rather than the details of how that is accomplished. > okay, this is a fair point :-) I just don't see, as of now at least, how we can > get to that in a way that's somewhat future-proof. It seems like we will > add more and more notifications until we can't maintain this anymore. So we need to look at the new/upcoming USB specs and understand the problem space, and how much of it we need to worry about right now in this scope. > With Type-C there's no port type anymore. What we used to refer as Host/Hub > Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single > type (let's call it type-c for short) which can provide profiles. These profiles > are negotiated using that new stack I pointed out above, not by checking > resistance on data lines or ID pin. Yup, which is a really cool feature and keeps us all employed too! :) This is one reason for attaching things to the USB stack, right now it does a limited negotiation but in future like you say there's more and more features coming. > Add to that the fact that the same power delivery pipe (the CC pins) can be used > to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan > :-) > In short, whatever we do, needs to consider coping with incoming requests from > userspace at a minimum because userspace will need control over the port > alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but > it'll be a pain to make a flexible/generic solution :-) That seems to make sense to me - anything the kernel is able to do autonomously for USB C should probably be pretty dumb. Older USB standards are already pretty dumb themselves (although inconsistently standardised). > > > The real difficulty here is coming up with an interface which we're agreeing to > > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > > > to userland, we will have to support it. > > To me that sounds like an argument for hiding things :) > the problem with hiding, though, is that once something new comes about, it > might not fit our current abstraction and it might be big PITA to support "old" > and new systems. Does the problem not get easier if we break it down to just the charger elements and realise that the USB C modes are going to have a lot more policy in them? > > > And this another reason I think a more granular approach is better, as it allows > > > us to easily add more pieces as they come along. > > OTOH it's more work for the system as a whole. > it's more work outside the kernel, yes. The total amount of work (kernel + > userspace) will be the same, regardless if you hide it in the kernel or in userspace. Like I say I'm worried about deployment issues for the split solutions making things harder than they should be.
On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote: > On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote: > > On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote: > > > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > > > > > > Things more difficult, if nothing else it means we need to get whatever > > > > > userspace component deployed in all relevant userspace stacks with > > > > > appropriate configuration in order to do things. > > > > > but that will be a thing for the kernel too. We will have to deploy this new > > > > kernel component in all relevant stacks with appropriate configuration in order > > > > to do things :-) No changes there. > > > > Sure, but it's one project which is developed entirely in the open - > > > that's a lot more manageable. > > > We can have a fully open source userland daemon too. Much like systemd, bluez, > > neard, and many, many others. Why wouldn't that be a thing ? > > The trouble is getting traction on adoption. Vendors have a habit of doing > things like finding problems and rather than reporting them deciding that the > open source solution isn't great and going and writing their own thing > (especially when they're in stealth mode) rather than talking to anyone. > Sometimes we manage to avoid that but it can be challenging, and often we only > even hear about the new thing after it's shipping and there's a lot of inertia > behind changing it. The kernel ABIs tend to be a lot sticker than userspace > things here. but this is not a solid enough argument to push anything into the kernel. > > Similar applies to power delivery charging profile themselves. Not all > > profiles are mandatory. Some are optional and that will be completely > > device/board specific. How an OEM implements that in the PCB is also > > completely board-specific :-) Some might have a dumb IC hardcoding some > > messages, some might have something largely more flexible. > > Right, and what I was trying to say was that it seems like the kernel ought to > be worrying about the board specific bits and userspace worrying about what to > do with those bits. we're not saying anything different in this front. I, too, want kernel to deal with certain details and expose notifications. Where we disagree is how granular should these notifications be and where they should be sent to. > > The things we've got with audio are a combination of tuning to taste and > > > (even more importantly) very different ideas about what you want to do > > > with an audio subsystem that influence how you set things up in a given > > > use case. > > > the same thing will happen with Type-C and power delivery. There won't be tuning > > to taste, of course :-) But there will be "very different ideas what what you > > want to do with" your charging methodology. > > Are those very different things about the use cases ("don't bother with > high speed data, get all the power you can" or whatever) or are they > about the details of the board? I'm pretty sure there will be interesting designs in the market. I'm pretty sure there will be type-C systems which won't support USB 3.1 for example, even though they'll support USB Power Delivery in its entirety. > > > > Actually, I was thinking about it in a more granular fashion. Kernel > > > > exposes a charger/power_supply, a few regulators, a UDC view and this > > > > single userspace daemon figures out how to tie those together. > > > > That sounds worrying - it means that new hardware support needs > > > supporting in every userspace (it seems inevitable that there will be at > > > least some reimplementations because that's fun) which gets old really > > > fast, and every userspace needs to understand every topology. > > > very true, but it's also true for a kernel solution. > > > It seems like you want it in the kernel because of it being convenient :-) > > Yes, definitely - my experience both in terms of watching how people > like handset vendors often work internally and in terms of getting > things adopted is that it's a lot easier if you can have one component > you need to update to handle new hardware rather than multiple ones that > need to be upgraded for things that are purely board differences. IMO, just because you have it in the kernel, it doesn't mean it'll be adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not used by Android (or has that changed ?) > > > > The view here isn't really a USB port, it's just a source of power. But how much > > > > power we can draw from it depends on, possibly, a ton of different chips and > > > > different notifications. > > > > Right, yes - it's the power input/output associated with the USB port. > > > The USB port is just the physical thing users can see so it's a good way > > > to label it. That could mean that we ought to move the aggregation bit > > > into the power supply code of course, but then a lot of the decisions > > > are going to be specific to USB. > > > in a sense, yes. But they can happen at a layer which knows nothing (or very > > little) about USB. Here's an example: > > > Not everybody follows USB Battery Charging class. Some chargers don't short > > D+/D- to signify they are a wall charger. Some will use different resistance > > values on the ID pin to tell you how much current you can draw from them. > > > From a PMIC (or something else) point of view, all it needs is a tiny current > > source and a an ADC, then it reports the ADC value to the upper layer. This > > really doesn't even need to know that it's using an ID pin, or whatever. > > This doesn't seem much different to things like the various GPIO to > subsystem X drivers we've got - we already have some for IIO type > things IIRC. that's really not what I was trying to point out :-) > > > How different are the end goals of these designs really going to be > > > though - that's more of the question for me? Part of what the kernel is > > > doing is hiding implementation details from userspace. I'd expect > > > userspace to be interested in things like the maximum current allowed in > > > or out in a given mode rather than the details of how that is accomplished. > > > okay, this is a fair point :-) I just don't see, as of now at least, how we can > > get to that in a way that's somewhat future-proof. It seems like we will > > add more and more notifications until we can't maintain this anymore. > > So we need to look at the new/upcoming USB specs and understand the not upcoming, it's already out there. > problem space, and how much of it we need to worry about right now in > this scope. > > > With Type-C there's no port type anymore. What we used to refer as Host/Hub > > Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single > > type (let's call it type-c for short) which can provide profiles. These profiles > > are negotiated using that new stack I pointed out above, not by checking > > resistance on data lines or ID pin. > > Yup, which is a really cool feature and keeps us all employed too! :) > This is one reason for attaching things to the USB stack, right now it > does a limited negotiation but in future like you say there's more and > more features coming. keep in mind that the same stack used to change a charging current, will also be used to tell the other end to mux those lines differently. > > Add to that the fact that the same power delivery pipe (the CC pins) can be used > > to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan > > :-) > > > In short, whatever we do, needs to consider coping with incoming requests from > > userspace at a minimum because userspace will need control over the port > > alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but > > it'll be a pain to make a flexible/generic solution :-) > > That seems to make sense to me - anything the kernel is able to do > autonomously for USB C should probably be pretty dumb. Older USB > standards are already pretty dumb themselves (although inconsistently > standardised). right. > > > > The real difficulty here is coming up with an interface which we're agreeing to > > > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > > > > to userland, we will have to support it. > > > > To me that sounds like an argument for hiding things :) > > > the problem with hiding, though, is that once something new comes about, it > > might not fit our current abstraction and it might be big PITA to support "old" > > and new systems. > > Does the problem not get easier if we break it down to just the charger > elements and realise that the USB C modes are going to have a lot more > policy in them? the thing with USB C is that it's not only for negotiating a charging power (with power delivery you're not necessarily tied to 5V, btw), that very stack will also be used to change to other alternate modes, and those can be anything: Thunderbolt, PCIe, DisplayPort, etc. > > > > And this another reason I think a more granular approach is better, as it allows > > > > us to easily add more pieces as they come along. > > > > OTOH it's more work for the system as a whole. > > > it's more work outside the kernel, yes. The total amount of work (kernel + > > userspace) will be the same, regardless if you hide it in the kernel or in userspace. > > Like I say I'm worried about deployment issues for the split solutions > making things harder than they should be. and I'm worried about us having too much inside the kernel and that making things harder than they should be :-)
On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote: > On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote: > > The trouble is getting traction on adoption. Vendors have a habit of doing > > things like finding problems and rather than reporting them deciding that the > > open source solution isn't great and going and writing their own thing > > (especially when they're in stealth mode) rather than talking to anyone. > > Sometimes we manage to avoid that but it can be challenging, and often we only > > even hear about the new thing after it's shipping and there's a lot of inertia > > behind changing it. The kernel ABIs tend to be a lot sticker than userspace > > things here. > but this is not a solid enough argument to push anything into the kernel. To my mind it is a concern when considering design - it's not the only consideration certainly but it should influence if or how we split things. > > > the same thing will happen with Type-C and power delivery. There won't be tuning > > > to taste, of course :-) But there will be "very different ideas what what you > > > want to do with" your charging methodology. > > Are those very different things about the use cases ("don't bother with > > high speed data, get all the power you can" or whatever) or are they > > about the details of the board? > I'm pretty sure there will be interesting designs in the market. I'm pretty sure > there will be type-C systems which won't support USB 3.1 for example, even > though they'll support USB Power Delivery in its entirety. That's sounding like generic USB stuff to me. > > Yes, definitely - my experience both in terms of watching how people > > like handset vendors often work internally and in terms of getting > > things adopted is that it's a lot easier if you can have one component > > you need to update to handle new hardware rather than multiple ones that > > need to be upgraded for things that are purely board differences. > IMO, just because you have it in the kernel, it doesn't mean it'll be > adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not > used by Android (or has that changed ?) That's always been a bit of a myth - most Android systems use runtime PM extensively when they're running, the discussion is really about the edge case where you're idling the system. The Android suspend behaviour is about the system idle case and is as much about providing a simple way to go into an idle state, especially bearing in mind that they have apps installed from the app store there, as anything else. It's the making sure things are idle part of things that's different. > > > okay, this is a fair point :-) I just don't see, as of now at least, how we can > > > get to that in a way that's somewhat future-proof. It seems like we will > > > add more and more notifications until we can't maintain this anymore. > > So we need to look at the new/upcoming USB specs and understand the > not upcoming, it's already out there. I assume there's ongoing work on further revisions to the spec though? > > Yup, which is a really cool feature and keeps us all employed too! :) > > This is one reason for attaching things to the USB stack, right now it > > does a limited negotiation but in future like you say there's more and > > more features coming. > keep in mind that the same stack used to change a charging current, will also be > used to tell the other end to mux those lines differently. Sure. > > Does the problem not get easier if we break it down to just the charger > > elements and realise that the USB C modes are going to have a lot more > > policy in them? > the thing with USB C is that it's not only for negotiating a charging power > (with power delivery you're not necessarily tied to 5V, btw), that very stack > will also be used to change to other alternate modes, and those can be anything: > Thunderbolt, PCIe, DisplayPort, etc. Surely these features have sufficient orthogonality to allow us to split things up and deal with some of the problems while providing spaces for future work?
Hi, On Mon, Oct 05, 2015 at 05:18:33PM +0100, Mark Brown wrote: > On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote: > > On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote: > > > > The trouble is getting traction on adoption. Vendors have a habit of doing > > > things like finding problems and rather than reporting them deciding that the > > > open source solution isn't great and going and writing their own thing > > > (especially when they're in stealth mode) rather than talking to anyone. > > > Sometimes we manage to avoid that but it can be challenging, and often we only > > > even hear about the new thing after it's shipping and there's a lot of inertia > > > behind changing it. The kernel ABIs tend to be a lot sticker than userspace > > > things here. > > > but this is not a solid enough argument to push anything into the kernel. > > To my mind it is a concern when considering design - it's not the only > consideration certainly but it should influence if or how we split > things. sure > > > > the same thing will happen with Type-C and power delivery. There won't be tuning > > > > to taste, of course :-) But there will be "very different ideas what what you > > > > want to do with" your charging methodology. > > > > Are those very different things about the use cases ("don't bother with > > > high speed data, get all the power you can" or whatever) or are they > > > about the details of the board? > > > I'm pretty sure there will be interesting designs in the market. I'm pretty sure > > there will be type-C systems which won't support USB 3.1 for example, even > > though they'll support USB Power Delivery in its entirety. > > That's sounding like generic USB stuff to me. > > > > Yes, definitely - my experience both in terms of watching how people > > > like handset vendors often work internally and in terms of getting > > > things adopted is that it's a lot easier if you can have one component > > > you need to update to handle new hardware rather than multiple ones that > > > need to be upgraded for things that are purely board differences. > > > IMO, just because you have it in the kernel, it doesn't mean it'll be > > adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not > > used by Android (or has that changed ?) > > That's always been a bit of a myth - most Android systems use runtime PM > extensively when they're running, the discussion is really about the > edge case where you're idling the system. The Android suspend behaviour > is about the system idle case and is as much about providing a simple > way to go into an idle state, especially bearing in mind that they have > apps installed from the app store there, as anything else. It's the > making sure things are idle part of things that's different. okay > > > > okay, this is a fair point :-) I just don't see, as of now at least, how we can > > > > get to that in a way that's somewhat future-proof. It seems like we will > > > > add more and more notifications until we can't maintain this anymore. > > > > So we need to look at the new/upcoming USB specs and understand the > > > not upcoming, it's already out there. > > I assume there's ongoing work on further revisions to the spec though? that'd be a correct assumption > > > Does the problem not get easier if we break it down to just the charger > > > elements and realise that the USB C modes are going to have a lot more > > > policy in them? > > > the thing with USB C is that it's not only for negotiating a charging power > > (with power delivery you're not necessarily tied to 5V, btw), that very stack > > will also be used to change to other alternate modes, and those can be anything: > > Thunderbolt, PCIe, DisplayPort, etc. > > Surely these features have sufficient orthogonality to allow us to split > things up and deal with some of the problems while providing spaces for > future work? yeah, might. As long as we keep that voice in our heads that things are likely to change.
On Fri, Oct 2, 2015 at 10:23 AM, Felipe Balbi via device-mainlining <device-mainlining@lists.linuxfoundation.org> wrote: [..] > The real difficulty here is coming up with an interface which we're agreeing to > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > to userland, we will have to support it. > Isn't this the main reason for not creating a user space ABI now? Running with the in-kernel hooks would allow us to get things working now, we can change it as we wish, we can explore the difficulties and we can create an ABI from that - if we need to - that might have a chance to work for the next 30 years. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
HI! > > + int ret; > > + > > + mutex_lock(&gadget->lock); > > + ret = raw_notifier_chain_unregister(&gadget->nh, nb); > > Greg, this is the kind of thing I wanted to avoid adding more of. > > I was wondering if you would accept subsystems using kdbus for > this sort of notification. I'm okay waiting for kdbus for another > couple merge windows (if we have to) before that's merged, but > if we take this raw notifier approach now, we will end up having > to support it forever. > > Also, because soon enough we will have to support USB Power Delivery > with Type C connector, this is bound to change in the coming months. > > Frankly, I wanted all of this to be decided in userland with the > kernel just providing notification and basic safety checks (we don't > want to allow a bogus userspace daemon frying anybody's devices). > > How would you feel about that ? So init=/bin/bash boot no longer provides machine that charges itself? That would be bad. Traditionally, hardware controls battery charging, and if hardware needs some help, we should do it in kernel, to mask the difference from userspace. Pavel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi! > > What's the advantage of pushing this to userspace? By the time we > > provide enough discoverability to enable userspace to configure itself > > it seems like we'd have enough information to do the job anyway. > > you're going to be dealing with a setup where each vendor does one thing > differently. Some will have it all in the SoC part of a single IP (dwc3 can be > configured that way), some will push it out to companion IC, some might even use > some mostly discrete setup and so on... > > We're gonna be dealing with a decision that involves information from multiple > subsystems (USB, regulator, hwmon, power supply to name a few). > > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's > just plain difficult. To make matters worse, N900 had two USB PHYs, one for > actual runtime use and another just for detecting the charger type on the other > end. N900 does charging from kernel these days. Not being able to boot generic distribution would be regression there... Pavel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi, Pavel Machek <pavel@ucw.cz> writes: > HI! > >> > + int ret; >> > + >> > + mutex_lock(&gadget->lock); >> > + ret = raw_notifier_chain_unregister(&gadget->nh, nb); >> >> Greg, this is the kind of thing I wanted to avoid adding more of. >> >> I was wondering if you would accept subsystems using kdbus for >> this sort of notification. I'm okay waiting for kdbus for another >> couple merge windows (if we have to) before that's merged, but >> if we take this raw notifier approach now, we will end up having >> to support it forever. >> >> Also, because soon enough we will have to support USB Power Delivery >> with Type C connector, this is bound to change in the coming months. >> >> Frankly, I wanted all of this to be decided in userland with the >> kernel just providing notification and basic safety checks (we don't >> want to allow a bogus userspace daemon frying anybody's devices). >> >> How would you feel about that ? > > So init=/bin/bash boot no longer provides machine that charges itself? > > That would be bad. Traditionally, hardware controls battery charging, > and if hardware needs some help, we should do it in kernel, to mask > the difference from userspace. this is a very valid point which I hadn't considered :-) Seems like kernel it is, no matter how easy or how difficult it gets. Mark, when can we try to have a discussion about how to get this upstream ? It seems like designing everything in the mailing list will just take forever. Any ideas ?
On Fri, Oct 09, 2015 at 04:17:27PM -0500, Felipe Balbi wrote: > Mark, when can we try to have a discussion about how to get this > upstream ? It seems like designing everything in the mailing list will > just take forever. Any ideas ? Can we take this off-list? I'm in the UK, Baolin is in China and I believe you're in the US so it might be a fun one to schedule...
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c index f660afb..4238fc3 100644 --- a/drivers/usb/gadget/udc/udc-core.c +++ b/drivers/usb/gadget/udc/udc-core.c @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep, } EXPORT_SYMBOL_GPL(usb_gadget_giveback_request); +int usb_gadget_register_notify(struct usb_gadget *gadget, + struct notifier_block *nb) +{ + int ret; + + mutex_lock(&gadget->lock); + ret = raw_notifier_chain_register(&gadget->nh, nb); + mutex_unlock(&gadget->lock); + + return ret; +} +EXPORT_SYMBOL_GPL(usb_gadget_register_notify); + +int usb_gadget_unregister_notify(struct usb_gadget *gadget, + struct notifier_block *nb) +{ + int ret; + + mutex_lock(&gadget->lock); + ret = raw_notifier_chain_unregister(&gadget->nh, nb); + mutex_unlock(&gadget->lock); + + return ret; +} +EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify); + /* ------------------------------------------------------------------------- */ /** @@ -226,6 +252,10 @@ static void usb_gadget_state_work(struct work_struct *work) struct usb_gadget *gadget = work_to_gadget(work); struct usb_udc *udc = gadget->udc; + mutex_lock(&gadget->lock); + raw_notifier_call_chain(&gadget->nh, gadget->state, gadget); + mutex_unlock(&gadget->lock); + if (udc) sysfs_notify(&udc->dev.kobj, NULL, "state"); } @@ -364,6 +394,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget, dev_set_name(&gadget->dev, "gadget"); INIT_WORK(&gadget->work, usb_gadget_state_work); + RAW_INIT_NOTIFIER_HEAD(&gadget->nh); + mutex_init(&gadget->lock); gadget->dev.parent = parent; #ifdef CONFIG_HAS_DMA diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c14a69b..755e8bc 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -609,6 +609,8 @@ struct usb_gadget { unsigned out_epnum; unsigned in_epnum; struct usb_otg_caps *otg_caps; + struct raw_notifier_head nh; + struct mutex lock; unsigned sg_supported:1; unsigned is_otg:1; @@ -1183,6 +1185,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget *gadget, /*-------------------------------------------------------------------------*/ +/** + * Register a notifiee to get notified by any attach status changes from + * the usb gadget + */ +int usb_gadget_register_notify(struct usb_gadget *gadget, + struct notifier_block *nb); + +/*-------------------------------------------------------------------------*/ + + +/* Unregister a notifiee from the usb gadget */ +int usb_gadget_unregister_notify(struct usb_gadget *gadget, + struct notifier_block *nb); + +/*-------------------------------------------------------------------------*/ + /* utility to set gadget state properly */ extern void usb_gadget_set_state(struct usb_gadget *gadget,
The usb charger framework is based on usb gadget. The usb charger need to be notified the state changing of usb gadget to confirm the usb charger state. Thus this patch adds a notifier mechanism for usb gadget to report a event to usb charger when the usb gadget state is changed. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++ include/linux/usb/gadget.h | 18 ++++++++++++++++++ 2 files changed, 50 insertions(+)