Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Tue, Mar 07 2017, Baolin Wang wrote: > On 3 March 2017 at 10:23, NeilBrown wrote: > >> >> I understand your reluctance to change drivers that you cannot test. >> An alternative it do change all the >> atomic_notifier_call_chain(.*notifier, >> calls that don't pass a pointer to vbus_draw to pass NULL, and to >> declare the passing of NULL to be deprecated (so hopefully people won't >> use it in new code). >> Then any notification callback that expects a current can just ignore >> calls where the pointer is NULL. > > I am afraid if it is enough to send out vbus draw information from USB > phy driver, for example you will miss super speed (900mA), which need > get the speed information from gadget driver. When the gadget driver determines that 900mA is available, it calls usb_phy_set_power() which calls the ->set_power() method on the usb_phy. The usb_phy the uses the notifier to inform all interested parties that 900mA is available. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v2 1/2] usb: phy: Introduce one extcon device into usb phy
On Thu, Mar 23 2017, Baolin Wang wrote:
> Usually usb phy need register one extcon device to get the connection
> notifications. It will remove some duplicate code if the extcon device
> is registered using common code instead of each phy driver having its
> own related extcon APIs. So we add one pointer of extcon device into
> usb phy structure, and some other helper functions to register extcon.
>
> Suggested-by: NeilBrown
> Signed-off-by: Baolin Wang
> ---
> Changes since v1:
> - Fix build errors with random config.
> ---
> drivers/usb/phy/Kconfig |6 +++---
> drivers/usb/phy/phy.c | 44
> include/linux/usb/phy.h |6 ++
> 3 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 61cef75..c9c61a8 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -4,6 +4,7 @@
> menu "USB Physical Layer drivers"
>
> config USB_PHY
> + select EXTCON
> def_bool n
>
> #
> @@ -116,7 +117,7 @@ config OMAP_OTG
>
> config TAHVO_USB
> tristate "Tahvo USB transceiver driver"
> - depends on MFD_RETU && EXTCON
> + depends on MFD_RETU
> depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be
> 'y'
> select USB_PHY
> help
> @@ -148,7 +149,6 @@ config USB_MSM_OTG
> depends on (USB || USB_GADGET) && (ARCH_QCOM || COMPILE_TEST)
> depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be
> 'y'
> depends on RESET_CONTROLLER
> - depends on EXTCON
> select USB_PHY
> help
> Enable this to support the USB OTG transceiver on Qualcomm chips. It
> @@ -162,7 +162,7 @@ config USB_MSM_OTG
> config USB_QCOM_8X16_PHY
> tristate "Qualcomm APQ8016/MSM8916 on-chip USB PHY controller support"
> depends on ARCH_QCOM || COMPILE_TEST
> - depends on RESET_CONTROLLER && EXTCON
> + depends on RESET_CONTROLLER
> select USB_PHY
> select USB_ULPI_VIEWPORT
> help
> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> index 98f75d2..baa8b18 100644
> --- a/drivers/usb/phy/phy.c
> +++ b/drivers/usb/phy/phy.c
> @@ -100,6 +100,41 @@ static int devm_usb_phy_match(struct device *dev, void
> *res, void *match_data)
> return *phy == match_data;
> }
>
> +static int usb_add_extcon(struct usb_phy *x)
> +{
> + int ret;
> +
> + if (of_property_read_bool(x->dev->of_node, "extcon")) {
> + x->edev = extcon_get_edev_by_phandle(x->dev, 0);
This is not what I meant to suggest.
This provides an extcon only for some phy devices. I meant that every
single usb_phy should always have an extcon.
So phy.c should probably call extcon_dev_allocate() and
extcon_dev_register() - or similar.
The driver then calls extcon_set_state() or similar on this extcon
device in whatever way might be appropriate.
For a phy driver like phy-qcom-8x16-usb it might just pass on
events from some separate extcon device.
For the (hypothetical?) device that Felipe suggests with separate
ID and VBUS extcons, it would pass on events from multiple different
extcons. i.e. it would act like a multiplexer.
Other drivers would do things from interrupt handlers, based on values
read from registers.
The important point is that each usb_phy doesn't get a pointer to some
separate extcon. Rather it has an extcon that it completely owns. The
name of the extcon is the same of the phy. The extcon should be seen as
a part of the phy, not a separate thing which provides service to the
phy.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
x27;vbus_draw' information, since we need to refactor and test
> these
> related phy drivers, power drivers or some mfd drivers, which is a
> huge workload.
You missed drivers/usb/musb/omap2430.c in you list, but that hardly
matters.
phy-ab8500-usb.c appears to send vbus_draw information.
I understand your reluctance to change drivers that you cannot test.
An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to
declare the passing of NULL to be deprecated (so hopefully people won't
use it in new code).
Then any notification callback that expects a current can just ignore
calls where the pointer is NULL.
The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c
It is the only driver which expects a 'gadget', and it doesn't really
need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to
pass NULL. When we can safely use the notifier to pass vbus_draw
information uniformly.
>
> (3) Still keep charger_type_show() API.
> Firstly I think we should combine all charger related information into one
> place for users, which is convenient.
convenience is very much a secondary issue.
> Secondly not only we get charger type from extcon, but also in some scenarios
> we can get charger type from USB controller driver, USB type-c driver, pmic
> driver, we should also need one place to export the charger type.
As I have said, all of these sources of information should feed into the
extcon.
There are ultimately two possible sources of information about the
current available from the usb port.
One is the physical properties of the cable, such as resistance of ID,
any short between D+ and D- etc. Being properties of the cable, they
should be reported through the extcon.
The other is information gathered during the USB protocol handshake.
For USB2, this is the requested current of the profile that the host
activates. This should be reported though the USB gadget device.
I don't know how USB3 and/or type-C work but I would be surprised if they
don't fit into the two cases above. If you think otherwise, please
surprise me. I'm always keen to learn.
If the extcon reports the type of cable detected, and the gadget reports
the result of any negotiation, then that is enough to determine the
charger type. It doesn't need to be more convenient than that.
Thanks,
NeilBrown
From 75a77246d2e9224f579ab996640a435a1c587d5f Mon Sep 17 00:00:00 2001
From: NeilBrown
Date: Fri, 3 Mar 2017 13:05:17 +1100
Subject: [PATCH] usb: gadget: pxa27x: change event handler to not expect
gadget.
We are planning to change the usb notifier to not pass a gadget,
as different drivers pass different things and consistency would
be nice.
The only driver to expect a gadget is pxa27x.
Unlike other drivers that register a usb notifier, pxa27x uses a static
notifier_block. Other drivers embed a notifer_block in their data
struct so that struct can be access from the notifer callback.
So change pxa27x to use as embedded notifer_block, and to use that
to find the target gadget.
With this, it can ignore that pointer that is passed. This will
allow us to change all calls that pass a gadget, to instead pass
NULL.
Signed-off-by: NeilBrown
---
drivers/usb/gadget/udc/pxa27x_udc.c | 14 ++
drivers/usb/gadget/udc/pxa27x_udc.h | 2 ++
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c
b/drivers/usb/gadget/udc/pxa27x_udc.c
index e1335ad5bce9..885a6c80fd08 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1669,7 +1669,8 @@ static int pxa_udc_vbus_draw(struct usb_gadget *_gadget,
unsigned mA)
static int pxa_udc_phy_event(struct notifier_block *nb, unsigned long action,
void *data)
{
- struct usb_gadget *gadget = data;
+ struct pxa_udc *udc = container_of(nb, struct pxa_udc, nb);
+ struct usb_gadget *gadget = &udc->gadget;
switch (action) {
case USB_EVENT_VBUS:
@@ -1683,10 +1684,6 @@ static int pxa_udc_phy_event(struct notifier_block *nb,
unsigned long action,
}
}
-static struct notifier_block pxa27x_udc_phy = {
- .notifier_call = pxa_udc_phy_event,
-};
-
static int pxa27x_udc_start(struct usb_gadget *g,
struct usb_gadget_driver *driver);
static int pxa27x_udc_stop(struct usb_gadget *g);
@@ -2504,8 +2501,9 @@ static int pxa_udc_probe(struct platform_device *pdev)
goto err;
}
+ udc->nb.notifier_call = pxa_udc_phy_event;
if (!IS_ERR_OR_NULL(udc->transceiver))
- usb_register_notifier(udc->transceiver, &pxa27x_udc_phy);
+ usb_register_notifier(udc->transceiver, &udc->nb);
retval = usb_add
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Dec 21 2016, Baolin Wang wrote:
> On 21 December 2016 at 11:48, NeilBrown wrote:
>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>
>>> Hi,
>>>
>>> On 21 December 2016 at 06:07, NeilBrown wrote:
>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>
>>>>> Hi Neil,
>>>>>
>>>>> On 3 November 2016 at 09:25, NeilBrown wrote:
>>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>>
>>>>>>
>>>>>>>> So I won't be responding on this topic any further until I see a
>>>>>>>> genuine
>>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>>> usb_register_notifier().
>>>>>>>
>>>>>>> Any better solution?
>>>>>>
>>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>>> the question I want to answer :-)
>>>>>>
>>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>> with USB connector types.
>>>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>> which both seem to suggest a standard downstream port. There is no
>>>>>> documentation describing how these relate, and no consistent practice
>>>>>> to copy.
>>>>>> I suspect the intention is that
>>>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>> the cable.
>>>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>>>> but it is not consistent.
>>>>>>
>>>>>> This policy should be well documented and possibly existing drivers
>>>>>> should be updated to follow it.
>>>>>>
>>>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>>>> They were recently removed from drivers/power/axp288_charger.c
>>>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>>>> Possibly they should be changed to names from the standard, or
>>>>>> possibly they should be renamed to identify the current they are
>>>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>>
>>>>> Now I am creating the new patchset with fixing and converting exist
>>>>> drivers.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>>> After checking, now all extcon drivers were following this rule.
>>>>
>>>> what about extcon-axp288.c ?
>>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>>> never sets EXTCON_USB.
>>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>
>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>
>>>>
>>>>>
>>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>>> Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>>> change.
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>>> There are no model that shows the slow charger should be 500mA
>>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>>> I think.
>>>>
>>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>>> anything.
>>>> The only place where the cable types are registered are in
>>>> extcon-max{14577,77693,77843,8
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Dec 21 2016, Baolin Wang wrote:
> Hi,
>
> On 21 December 2016 at 06:07, NeilBrown wrote:
>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>
>>> Hi Neil,
>>>
>>> On 3 November 2016 at 09:25, NeilBrown wrote:
>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>
>>>>
>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>> usb_register_notifier().
>>>>>
>>>>> Any better solution?
>>>>
>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>> the question I want to answer :-)
>>>>
>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>> with USB connector types.
>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>> which both seem to suggest a standard downstream port. There is no
>>>> documentation describing how these relate, and no consistent practice
>>>> to copy.
>>>> I suspect the intention is that
>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>> the cable.
>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>> but it is not consistent.
>>>>
>>>> This policy should be well documented and possibly existing drivers
>>>> should be updated to follow it.
>>>>
>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>> They were recently removed from drivers/power/axp288_charger.c
>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>> Possibly they should be changed to names from the standard, or
>>>> possibly they should be renamed to identify the current they are
>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>
>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>
>> Thanks!
>>
>>>
>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>> After checking, now all extcon drivers were following this rule.
>>
>> what about extcon-axp288.c ?
>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>> never sets EXTCON_USB.
>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>
> Ha, sorry, I missed these 2 files, and I will fix them.
>
>>
>>>
>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>> Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>> change.
>>
>> Agreed.
>>
>>>
>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>> There are no model that shows the slow charger should be 500mA
>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>> I think.
>>
>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>> anything.
>> The only place where the cable types are registered are in
>> extcon-max{14577,77693,77843,8997}.c
>>
>> In each case, the code strongly suggests that the meaning is that "slow"
>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>
>> With names like "fast" and "slow", any common changer framework cannot
>> make use of these cable types as the name doesn't mean anything.
>> If the names were changed to 500MA/1A, then common code could reasonably
>> assume how much current can safely be drawn from each.
>
> As I know, some fast charger can be drawn 5A, then do we need another
> macro named 5A? then will introduce more macros in future, I am not
> true this is helpful.
It isn't really a question of what the charger can provide. It is a
question of what the cable reports to the phy.
Unfortunately I cannot find any datasheets on line to verify how these
devices work, but the code seems to suggest that they can detect and
report special charger types that provide 500mA and 1A respectively.
If the hardware exists that reports the functionality, it make sense to
use it. Hopefully new hardware will follow the USB BC spec, so further
special cases won't be needed.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Tue, Dec 20 2016, Baolin Wang wrote:
> Hi Neil,
>
> On 3 November 2016 at 09:25, NeilBrown wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>>
>>>> So I won't be responding on this topic any further until I see a genuine
>>>> attempt to understand and resolve the inconsistencies with
>>>> usb_register_notifier().
>>>
>>> Any better solution?
>>
>> I'm not sure exactly what you are asking, so I'll assume you are asking
>> the question I want to answer :-)
>>
>> 1/ Liase with the extcon developers to resolve the inconsistencies
>> with USB connector types.
>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>> which both seem to suggest a standard downstream port. There is no
>> documentation describing how these relate, and no consistent practice
>> to copy.
>> I suspect the intention is that
>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>> the cable.
>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>> would normally appear with EXTCON_USB_HOST (I think).
>> Some drivers follow this model, particularly extcon-max14577.c
>> but it is not consistent.
>>
>> This policy should be well documented and possibly existing drivers
>> should be updated to follow it.
>>
>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>> They were recently removed from drivers/power/axp288_charger.c
>> which is good, but are still used in drivers/extcon/extcon-max*
>> Possibly they should be changed to names from the standard, or
>> possibly they should be renamed to identify the current they are
>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>
> Now I am creating the new patchset with fixing and converting exist drivers.
Thanks!
>
> I did some investigation about EXTCON subsystem. From your suggestion:
> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
> After checking, now all extcon drivers were following this rule.
what about extcon-axp288.c ?
axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
never sets EXTCON_USB.
Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>
> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
> Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
> change.
Agreed.
>
> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
> There are no model that shows the slow charger should be 500mA
> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
> I think.
Leaving the names a SLOW/FAST is less useful as those names don't *mean*
anything.
The only place where the cable types are registered are in
extcon-max{14577,77693,77843,8997}.c
In each case, the code strongly suggests that the meaning is that "slow"
means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
With names like "fast" and "slow", any common changer framework cannot
make use of these cable types as the name doesn't mean anything.
If the names were changed to 500MA/1A, then common code could reasonably
assume how much current can safely be drawn from each.
>
> From above investigation, I did not find anything need to be changed
> now. What do you think? Thanks.
>
As above, I think changes are needed.
I particularly highlight:
>> This policy should be well documented and possibly existing drivers
>> should be updated to follow it.
The documentation is key. A usb charger framework needs to depend on
correct information from extcon, and currently there is no clear
documentation on how a USB phy should signal the charger type.
There isn't a lot to say: primarily that both the EXTCON_USB* and
EXTCON_CGH_USB* types should be provided together. Each does not imply
the other in any way. But it still needs to be documented.
Thanks,
NeilBrown
>>
>> 2/ Change all usb phys to register an extcon and to send appropriate
>>notifications. Many already do, but I don't think it is universal.
>>It is probable that the extcon should be registered using common code
>>instead of each phy driver having its own
>>extcon_get_edev_by_phandle()
>>or whatever.
>>If the usb phy driver ne
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Sat, Nov 26 2016, Mark Brown wrote: > [ Unknown signature status ] > On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote: > >> I agree that the question of where the responsibility for information >> aggregation lies is open for discussion. If fact all details on how >> things should work are always open for discussion. >> I don't agree that this is the main different between our positions, >> though I can see how you might get that impression. > >> You could even fix them so they look *exactly* like the notifiers that >> Baolin is proposing. This is my key point. It is not the end result >> that I particularly object to (though I do object to some details). It > > Ah, OK. This really hadn't been at all clear - both Baolin and I had > the impression that the it was both that were blockers for you. What > were the details here? I don't really like the idea of a separate "usb charger" object. It looks too much like a "midlayer" and they tend to get in the way. But if a convincing case could be made that changing from the current design to that aspect of the proposed design brings measurable benefits, then I would certainly assess that case on its merits. No such case was made, and the patchset didn't seem to even acknowledge the existing design. When I said "I do object to some details" it was details of the end result, not details of what took responsibility of information aggregation (in case that wasn't clear). Those details were everything that duplicated existing functionality, or ignored existing functionality, or was simply unworkable. e.g. the lack of proper integration with extcon, the new sysfs attributes, the name-lookup mechanism. Probably others. > >> is the process of getting to the end result that I don't like. If the >> current system doesn't work and something different is needed, then the >> correct thing to do is to transform the existing system into something >> new that works better. This should be a clear series of steps. Each > > Sometimes there's something to be said for working out what we want > things to look like before setting out to make these gradual > refactorings and sometimes the refactorings are just more pain than > they're worth, especially when they go across subsystems. In this case > I do worry about the cross subsystem aspect causing hassle, it may be > more practical to do anything that represents an interface change by > adding the new interface, converting the users to it and then removing > the old interface. Yes, you need a clear vision of the goal. You also need a clear vision of the starting point. There was no evidence of the latter. Yes, sometimes you need to create a new thing and transition users over, then discard the old. There was no discarding of the old. > > At the very least the series should grow to incorporate conversion of > the existing users though. Baolin, I think this does need adding to the > series but probably best to think about how to do it - some of Neil's > suggestions for incremental steps do seem like they should be useful > for organizing things here, probably we can get some things that can be > done internally within individual drivers merged while everything else > is under discussion. I would be very encouraged to see those simple things done first! Seeing the series grow isn't much fun, but seeing preliminary work land certainly is. > >> But I think here my key point got lost too, in part because it was hard >> to refer to an actual instance. >> My point was that in the present patch set, the "usb charger" is given >> a name which is dependant on discovery order, and only supports >> lookup-by-name. This cannot work. > > There's two bits here: one is the way names are assigned and the other > is the lookup by name. I agree that the lookup by name isn't > particularly useful as things stand, that could just be dropped until > some naming mechanism is added. We'd be more likely to use phandles in > DT systems, I don't know what ACPI systems would look like but I guess > it'd be something similar. > >> If they supported lookup by phy-name or lookup-by-active (i.e. "find me >> any usb-charger which has power available"), or look up by some other >> attribute, then discover-order naming could work. But the only >> lookup-mechanism is by-name, and the names aren't reliably stable. So >> the name/lookup system proposed cannot possibly do anything useful >> with more than one usb_charger. > > Baolin, I think adding a DT binding and lookup mechanism makes sense > here - do you agree? We already have a lookup mechanism for a battery charger to find the phy that it gets current from: devm_usb_get_phy_by_phandle() (or even devm_usb_get_phy() if there is known to only be one phy). We would need a case to be made that the existing mechanism cannot be used before we consider "adding a DT binding and lookup mechanism". Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Tue, Nov 22 2016, Mark Brown wrote: > [ Unknown signature status ] > On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote: >> On Thu, Nov 17 2016, Mark Brown wrote: > >> > To me that's pretty much what's being done here, the code just happens >> > to sit in USB instead but fundamentally it's just a blob of helper code, >> > you could replace the notifier with a callback if that's the big concern >> > here. > >> It is a lot more than "just a blob of helper code". It duplicates >> existing infrastructure instead of fixing and using the >> infrastructure but I've said all this before. Repeatedly. > > My read on that is that the question of what we want to be responsible > for aggregating the information about what power the system is allowed > to draw from a given USB port hasn't been resolved yet and that apart > from that you're fairly close. It seems to me like that's really what > the difference between your two positions is. Fixing the existing > notifiers implies that things have to be aggregated in the power supply > drivers but Baolin is proposing doing that in the USB code instead. It > does seem at least worth considering if that's a good idea since the > current situation doesn't look terribly thought through. I agree that the question of where the responsibility for information aggregation lies is open for discussion. If fact all details on how things should work are always open for discussion. I don't agree that this is the main different between our positions, though I can see how you might get that impression. I don't agree that "Fixing the existing notifiers implies that things have to be aggregated in the power supply drivers". That would be the simplest way to fix them, but not the only way. You could fix them so that the usb driver sends notifications instead of the phy, and you could fix them so that the information contained in the notification being a power range instead the current ad-hoc inconsistent set of information. You could even fix them so they look *exactly* like the notifiers that Baolin is proposing. This is my key point. It is not the end result that I particularly object to (though I do object to some details). It is the process of getting to the end result that I don't like. If the current system doesn't work and something different is needed, then the correct thing to do is to transform the existing system into something new that works better. This should be a clear series of steps. Each one makes a small, well defined, change. Maybe it adds a new feature. Maybe it refactors code without changing functionality. Maybe it fixes a bug. Maybe it moves the responsibility for an action from *here* to *there*. Each step is clearly explained and convincingly justified. This doesn't have to be a long justification. Sometimes a single sentence or a short paragraph is plenty. Sometimes the change is so obviously an improvement that no explanation is necessary. Changing one step at a time make it easier to ensure that no functionality is lost and that each change is actually needed. If the patch series fixed the existing code and transformed it into something better and more powerful, then it would be a lot easier to have sensible discussions about individual patches, about which there might be disagreement. > > There are a whole bunch of things that need to be sorted out whatever > the decision is like the extcon related cleanups you mentioned in your > mail the other day (steps 1 and 2), it seems like those could be moved > forwards independently. Agreed. > > By the way it occurred to me recently that we have a use case for > multiple USB ports that could supply power - USB C. Things with more > than one port like things in a laptop form factor are going to want to > be able to use all of them interchangably for power support (likely only > one at a time, at least initially, but still more than one port). Thanks! It is so much easier to discusses these sorts of things where there is a concrete reference point. I wonder if each port would have its own current regulator, or if there would be a single central one. Maybe there would be a switch that allowed power to flow in only from one port. Would each "charger" need to get notifications from "its" port, or would there be a single "charger" which got notifications from the power-switch, which in-turn got notifications from each port? Or would the battery-charger driver want to register with every port, receive notifications, and be able to turn each one on or off? Without concrete details of actual hardware, we can only guess. But I think here my key point got lost too, in part because it was hard to refer to an actual instance.
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Thu, Nov 17 2016, Mark Brown wrote: > [ Unknown signature status ] > On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote: >> On Mon, Nov 14 2016, Mark Brown wrote: > >> > Conflating the two seems like the whole point here. We're looking for >> > something that sits between the power supply code and the USB code and >> > tells the power supply code what it's allowed to do which is the result >> > of a combination of physical cable detection and USB protocol. It seems >> > reasonable that extcon drivers ought to be part of this but it doesn't >> > seem like they are the whole story. > >> I don't think "between the power supply code and the USB code" is where >> this thing sits. I think it sits inside the power-supply driver. >> We already have extcon which sits between the phy and the power_supply >> code, and the usb_notifier which sits between the USB code and the >> power supply code. We don't need another go-between. > > ... > >> correct determinations and set the current limits itself. All this >> could be done entirely internally, without the help of any new >> subsystem. >> Do you agree? > >> Clearly doing it that way would result in lots of code duplication and >> would mean that each driver probably gets its own private set of bugs, >> but it would be possible. Just undesirable. > > I think this is the key here - the fact that it's technically possible > to implement doesn't really matter if it's sufficiently fiddly and non > obvious that nobody is actually joining everything up (bits are done > intermittently but not as a whole as far as I can see). > >> So yes, it makes perfect to provide common code which handles the >> registrations, and captures the events, and translates the different >> events into current levels and feeds those back to the driver. This >> isn't some new subsystem, this is just a resource, provided by a >> library, that power drivers can allocate and initialize if the want to. > > To me that's pretty much what's being done here, the code just happens > to sit in USB instead but fundamentally it's just a blob of helper code, > you could replace the notifier with a callback if that's the big concern > here. It is a lot more than "just a blob of helper code". It duplicates existing infrastructure instead of fixing and using the infrastructure but I've said all this before. Repeatedly. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Mon, Nov 14 2016, Mark Brown wrote: > On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote: >> On Thu, Nov 10 2016, Baolin Wang wrote: > >> > Fourth, we need integrate all charger plugin/out >> > event in one framework, not from extcon, maybe type-c in future. > >> Why not extcon? Given that a charger is connected by an external >> connector, extcon seems like exactly the right thing to use. > >> Obviously extcon doesn't report the current that was negotiated, but >> that is best kept separate. The battery charger can be advised of the >> available current either via extcon or separately via the usb >> subsystem. Don't conflate the two. > > Conflating the two seems like the whole point here. We're looking for > something that sits between the power supply code and the USB code and > tells the power supply code what it's allowed to do which is the result > of a combination of physical cable detection and USB protocol. It seems > reasonable that extcon drivers ought to be part of this but it doesn't > seem like they are the whole story. I don't think "between the power supply code and the USB code" is where this thing sits. I think it sits inside the power-supply driver. We already have extcon which sits between the phy and the power_supply code, and the usb_notifier which sits between the USB code and the power supply code. We don't need another go-between. If we have extcon able to deliver reliable information about cable type, and if with have the usb notifier able to deliver reliable information about negotiated current, and if the power supply manager is able to register with the correct extcon and the correct usb notifier, then the power supply manager *could* handle all the notifications and make the correct determinations and set the current limits itself. All this could be done entirely internally, without the help of any new subsystem. Do you agree? Clearly doing it that way would result in lots of code duplication and would mean that each driver probably gets its own private set of bugs, but it would be possible. Just undesirable. So yes, it makes perfect to provide common code which handles the registrations, and captures the events, and translates the different events into current levels and feeds those back to the driver. This isn't some new subsystem, this is just a resource, provided by a library, that power drivers can allocate and initialize if the want to. To quote myself: > 5/ Now that all cable connection notifications are sent over extcon and >all vbus_draw notifications are sent over the usb_phy notifier, write >some support code that a power supply client can use to be told what >power is available. >e.g. a battery charger driver would call: >register_power_client(.) >or similar, providing a phandle (or similar) for the usb phy and a >function to call back when the available current changes (or maybe a >work_struct containing the function pointer) > >register_power_client() would then register with extcon and separately > with the usb_phy notifier. When the different events arrive it >calculates what ranges of currents are expected and calls the >call-back function with those details. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Thu, Nov 10 2016, Baolin Wang wrote: > Hi > > On 8 November 2016 at 04:36, NeilBrown wrote: >> On Mon, Nov 07 2016, Baolin Wang wrote: >> >>> On 3 November 2016 at 09:25, NeilBrown wrote: >>>> On Tue, Nov 01 2016, Baolin Wang wrote: >>> >>> I agree with your most opinions, but these are optimization. >> >> I see them as bug fixes, not optimizations. >> >>> Firstly I >>> think we should upstream the USB charger driver. >> >> I think you missed the point. The point is that we don't *need* your >> "USB charger driver" because all the infrastructure we need is *already* >> present in the kernel. It is buggy and not used uniformly, and could >> usefully be polished and improved. But the structure is already >> present. >> >> If everyone just added new infrastructure when they didn't like, or >> didn't understand, what was already present, the kernel would become >> like the Mad Hatter's tea party, full of dirty dishes. >> >>> What I want to ask is >>> how can we notify power driver if we don't set the >>> usb_register_notifier(), then I think you give the answer is: power >>> driver can register by 'struct usb_phy->notifier'. But why usb phy >>> should notify the power driver how much current should be drawn, and I >>> still think we should notify the current in usb charger driver which >>> is better, and do not need to notify current for power driver in usb >>> phy driver. >> >> I accept that it isn't clear that the phy *should* be involved in >> communicating the negotiated power availability, but nor is it clear >> that it shouldn't. The power does travel through the physical >> interface, so physically it plays a role. >> >> But more importantly, it already *does* get told (in some cases). >> There is an interface "usb_phy_set_power()" which exists explicitly to >> tell the phy what power has been negotiated. Given that infrastructure >> exists and works, it make sense to use it. >> >> If you think it is a broken design and should be removed, then fine: >> make a case for that. Examine the history. Make sure you know why it >> is there (or make sure that information cannot be found), and then >> present a case as to why it should be removed and replaced with >> something else. But don't just leave it there and pretend it doesn't >> exist and create something similar-but-different and hope people will >> know why yours is better. That way lies madness. > > Like Peter said, it is not only PHY can detect the USB charger type, > which means there are other places can detect the charger type. If I understand Peter's example correctly, it shows a configuration where the USB PHysical interface is partly implemented in the SOC and partly in the PMIC. I appreciate that would make it more challenging to implement a PHY driver, but there is no reason it should impact anything outside of the PHY. > Second, some controller need to detect the charger type manually which > USB phy did not support. "manually" is an odd term to use in this context. I think you mean that to detect the charger type you need to issue some command to the hardware and wait for it to respond, then assess the response. That isn't at all surprising. The charger type is detected by measuring resistance between ID and GND, which may require setting up a potential and activating ADCs to measure the voltage. This can all be done internally to the phy driver. Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for twl4030, though it never got upstream). The code for the imx7d does look more complex, but not intrinsically different. > Third, it did not handle what current should > be drawn in USB phy. The standards define that. The extcon just reports the cable type. Certainly it would be sensible to provide a library function to translate from cable type to current range. You don't need a new subsystem to do that, just a library function. > Fourth, we need integrate all charger plugin/out > event in one framework, not from extcon, maybe type-c in future. Why not extcon? Given that a charger is connected by an external connector, extcon seems like exactly the right thing to use. Obviously extcon doesn't report the current that was negotiated, but that is best kept separate. The battery charger can be advised of the available current either via extcon or separately via the usb subsystem. Don't conflate the two. > In a >
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Tue, Nov 08 2016, Peter Chen wrote: > On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote: >> >> >> >> 2/ Change all usb phys to register an extcon and to send appropriate >>notifications. Many already do, but I don't think it is universal. >>It is probable that the extcon should be registered using common code >>instead of each phy driver having its own >>extcon_get_edev_by_phandle() >>or whatever. >>If the usb phy driver needs to look at battery charger registers to >>know what sort of cable was connected (which I believe is the case >>for the chips you are interested in), then it should do that. > > Not only USB PHY to register an extcon, but also for the drivers which > can detect USB charger type, it may be USB controller driver, USB type-c > driver, pmic driver, and these drivers may not have an extcon device > since the internal part can finish the vbus detect. Can you point to an example of the sort of hardware/driver you are referring to, preferably in the mainline kernel. Concrete examples make this sort of thing much easier to understand. I think I would argue that if some piece of hardware can detect the USB charger type, then that piece of hardware is part of the "USB PHY", even if the hardware manufacturer has chosen to partition the register set in a way which doesn't make that obvious. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Mon, Nov 07 2016, Baolin Wang wrote: > On 3 November 2016 at 09:25, NeilBrown wrote: >> On Tue, Nov 01 2016, Baolin Wang wrote: > > I agree with your most opinions, but these are optimization. I see them as bug fixes, not optimizations. > Firstly I > think we should upstream the USB charger driver. I think you missed the point. The point is that we don't *need* your "USB charger driver" because all the infrastructure we need is *already* present in the kernel. It is buggy and not used uniformly, and could usefully be polished and improved. But the structure is already present. If everyone just added new infrastructure when they didn't like, or didn't understand, what was already present, the kernel would become like the Mad Hatter's tea party, full of dirty dishes. > What I want to ask is > how can we notify power driver if we don't set the > usb_register_notifier(), then I think you give the answer is: power > driver can register by 'struct usb_phy->notifier'. But why usb phy > should notify the power driver how much current should be drawn, and I > still think we should notify the current in usb charger driver which > is better, and do not need to notify current for power driver in usb > phy driver. I accept that it isn't clear that the phy *should* be involved in communicating the negotiated power availability, but nor is it clear that it shouldn't. The power does travel through the physical interface, so physically it plays a role. But more importantly, it already *does* get told (in some cases). There is an interface "usb_phy_set_power()" which exists explicitly to tell the phy what power has been negotiated. Given that infrastructure exists and works, it make sense to use it. If you think it is a broken design and should be removed, then fine: make a case for that. Examine the history. Make sure you know why it is there (or make sure that information cannot be found), and then present a case as to why it should be removed and replaced with something else. But don't just leave it there and pretend it doesn't exist and create something similar-but-different and hope people will know why yours is better. That way lies madness. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Tue, Nov 01 2016, Baolin Wang wrote: >> So I won't be responding on this topic any further until I see a genuine >> attempt to understand and resolve the inconsistencies with >> usb_register_notifier(). > > Any better solution? I'm not sure exactly what you are asking, so I'll assume you are asking the question I want to answer :-) 1/ Liase with the extcon developers to resolve the inconsistencies with USB connector types. e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP" which both seem to suggest a standard downstream port. There is no documentation describing how these relate, and no consistent practice to copy. I suspect the intention is that EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of the cable, while EXTCON_CHG_USB* indicate the power capabilities of the cable. So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST (I think). Some drivers follow this model, particularly extcon-max14577.c but it is not consistent. This policy should be well documented and possibly existing drivers should be updated to follow it. At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW and EXTCON_CHG_USB_FAST. These names don't mean much. They were recently removed from drivers/power/axp288_charger.c which is good, but are still used in drivers/extcon/extcon-max* Possibly they should be changed to names from the standard, or possibly they should be renamed to identify the current they are expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A 2/ Change all usb phys to register an extcon and to send appropriate notifications. Many already do, but I don't think it is universal. It is probable that the extcon should be registered using common code instead of each phy driver having its own extcon_get_edev_by_phandle() or whatever. If the usb phy driver needs to look at battery charger registers to know what sort of cable was connected (which I believe is the case for the chips you are interested in), then it should do that. 3/ Currently some USB controllers discover that a cable was connected by listening on an extcon, and some by registering for a usb_notifier (described below) ... though there seem to only be 2 left which do that. Now that all USB phys send connection information via extcon (see 2), the USB controllers should be changed to all find out about the cable using extcon. 4/ struct usb_phy contains: /* for notification of usb_phy_events */ struct atomic_notifier_head notifier; This is used inconsistently. Sometimes the argument passed is NULL, sometimes it is a pointer to 'vbus_draw' - the current limited negotiated via USB, sometimes it is a pointer the the gadget though as far as I can tell, that last one is never used. This should be changed to be consistent. This notifier is no longer needed to tell the USB controller that a cable was connected (extcon now does that, see 3) so it is only used to communicate the 'vbus_draw' information. So it should be changed to *only* send a notification when vbus_draw is known, and it should carry that information. This should probably be done in common code, and removed from individual drivers. 5/ Now that all cable connection notifications are sent over extcon and all vbus_draw notifications are sent over the usb_phy notifier, write some support code that a power supply client can use to be told what power is available. e.g. a battery charger driver would call: register_power_client(.) or similar, providing a phandle (or similar) for the usb phy and a function to call back when the available current changes (or maybe a work_struct containing the function pointer) register_power_client() would then register with extcon and separately with the usb_phy notifier. When the different events arrive it calculates what ranges of currents are expected and calls the call-back function with those details. 6/ Any battery charger that needs to know the available current can now call register_power_client() and get the information delivered. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
terface design perspective, the number of ICs doesn't matter at all. The interface presented, both within the kernel and out to user-space, should be consistent. Every USB port should present with an EXTCON, and if it can detect types of cables, those cable types should be visible in the extcon. > >> >> And I just noticed you have a ->charger_detect() too, which seems >> identical to ->get_charger_type(). There is no documentation >> explaining the difference. > > I think the kernel doc have explained that, but I like to explain it > again. Since we can detect the charger by software or hardware (like > PMIC), if you need to detect your charger type by software, then you > can implement this callback, you can refer to Jun's patch: > http://www.spinics.net/lists/linux-usb/msg139808.html The kernel-doc says: + * @get_charger_type: User can get charger type by implementing this callback. + * @charger_detect: Charger detection method can be implemented if you need to + * manually detect the charger type. To me, that doesn't say anything useful. What is the difference between "get charger type" and "manually detect the charger type" ?? I don't want to have to refer to some extra set of patches to guess how something is supposed to work. It needs to be clearly and unambiguously documented. However, I'm very quickly losing interest in this whole debate, partly because it misses a key point. As I have said, I strongly feel that resolving the current inconsistencies in the use of usb_register_notifier() is an important preliminary to resolve this issue, as that is *already* sometimes used to communication available current. So I won't be responding on this topic any further until I see a genuine attempt to understand and resolve the inconsistencies with usb_register_notifier(). NeilBrown signature.asc Description: PGP signature
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
ready does that, and duplication is confusing and pointless. And I just noticed you have a ->charger_detect() too, which seems identical to ->get_charger_type(). There is no documentation explaining the difference. 5/ There is no convincing example usage of this framework. wm8931x_power.c just scratches the surface. If it is so good, it should be easy to convert a lot of other drivers over to it. If you did that it would be much easier to see how it works and what the strengths/weaknesses were. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Oct 05 2016, Felipe Balbi wrote: > Hi Baolin, > > Baolin Wang writes: >>>> But you do! >>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw. >>>> Your patch passes that to usb_charger_set_cur_limit_by_type() >>>> which calls __usb_charger_set_cur_limit_by_type() which will set the >>>> cur_limit for whichever type uchger->type currently is. >>>> >>>> So when it is not relevant, your code *does* set some current limit. >>> >>> Suppose the charger type is DCP(it is not relevant to the mA number >>> from the USB configuration ), it will not do the USB enumeration, then >>> no USB configuration from host to set current. >> >> From the talking, there are some issues (thanks for Neil's comments) >> need to be fixed as below: >> 1. Need to add the method getting charger type from extcon subsystem. >> 2. Need to remove the method getting charger type from power supply. >> 3. There are still some different views about reporting the maximum >> current or minimum current to power driver. >> >> Now the current v16 patchset can work well on my Spreadtrum platform >> and Jun's NXP platform, if you like to apply this patchset then I can I'm really curious how much testing this has had. Have you actually plugged in different cable types (SDP DCP DCP ACA) and has each one been detected correctly? Because I cannot see how that could happen with the code you have posted. >> send out new patches to fix above issues. If you don't like that, I >> can send out new version patchset to fix above issues. Could you give >> me some suggestions what should I do next step? Thanks. > > Merge window just opened, nothing will happen for about 2 weeks. How > about you send a new version after merge window closes and we go from > there? Fixing 1 and 2 is needed. 3 we need to consider more > carefully. Perhaps report both minimum and maximum somehow? > > Neil, comments? This probably seems a bit harsh, but I really think the current patchset should be discarded and the the project started again with a clear vision of what is required. What we currently have is too confused. To respond to the points: >> 1. Need to add the method getting charger type from extcon subsystem. Yes. This should be the only way to get the charger type. >> 2. Need to remove the method getting charger type from power supply. Also need to remove the ->get_charger_type() method as there is no credible use-case for this. >> 3. There are still some different views about reporting the maximum >> current or minimum current to power driver. I think those were resolved. There was some confusion over whether a particular power manager wanted to be told the maximum or the minimum, but I think both have a clear use case in different hardware. Also: We don't want another notifier_chain. The usb_notifier combined with the extcon notifier are sufficient. Possibly it would be sensible to replace the usb notifier with a new new notifier chain, but don't add something without first cleaning up what is there. Also: resolve the question of whether it could ever make sense to have more than one "usb_charger" in a system. If it doesn't, make it an obvious singleton. If it does, make it clear how the correct usb_charger is chosen. Also: think very carefully before exposing any details through sysfs. Some of the details are already visible, either in sys/class/extcon or sys/class/power_supply. Don't duplicate without good reason. NeilBrown > > -- > balbi signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Sep 14 2016, Mark Brown wrote: > TI do a lot of the more software managed chargers (which I suspect are > the main thing Felipe will have looked at) if that's what you're > referring to here? The device is implementing pretty much the algorithm > you're describing in that e-mail so I'm a bit confused as to what you're > saying here. Ah my mistake, sorry. When earlier you said: > It's a > current limiter intended to sit in line with the USB power lines to > ensure the system doesn't go over the maximum current draw (and also > integrates with the power source selection logic the chip has to pick > the best available power source for the system). I assumed that all it did was limit the current to number given. If it also limits the current to ensure that voltage doesn't drop unduly, then I agree with your assertion that it just needs to be told the upper limit. I hope you'll agree that other drivers might need to know the lower limit, so reporting both to all drivers is sensible. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Wed, Sep 14 2016, Mark Brown wrote: > [ Unknown signature status ] > On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote: >> On Mon, Sep 12 2016, Mark Brown wrote: > >> > That's not actually 100% clear to me - for what the wm831x is doing it >> > probably *does* want the higher limit. This is a system inflow limit >> > (as it should be for this), at least the charger will adapt to voltage >> > variations though other users in the system are much less likely to do >> > so. > >> Not having very much electrical engineering background, I cannot say for >> sure what will happen, but it seems likely that once the voltage drops >> much below 4.75V, the charger won't be operating at peak efficiency, >> which would be a waste. >> I can easily imagine that the hardware would switch off at some voltage >> level, rather than just making do with what is there. >> So I'm skeptical of this approach, but I'm open to being corrected by >> someone more knowledgeable than I. > > Yes, the idea is that the charger will back off charging and stop > entirely if the rest of the system is consuming too much power to allow > it to continue effectively. The same thing happens with wall power, if > a wall supply isn't able to power the charger (eg, because the rest of > the system is running flat out) it'll have to cope with that. Maybe you are correct. I don't find your argument convincing, but maybe that is because I don't want to... Some facts though: 1/ I had a report once from someone whose device stopped charging because it was pulling more current than the charger could supply. The voltage dropped below the 3.5V (I think) that the battery charging hardware needed, so it switched off. It wouldn't switch back on again until explicitly told too. It would then overload the charger again and switch off. Changing the code to put a lower limit on the current allowed the battery to be charged. So empirical evidence suggests that the lower number should be used. 2/ I hoped that Battery Charging Specification Revision 1.2 December 7, 2010 would say something definite, but I cannot find it. However, "note 1" to "Table 5-2 Currents" says: 1) The maximum current is for safety reasons, as per USB 2.0 section 7.2.1.2.1. Which seems to say the maximum is just for safety, implying that the minimum is the important value. 3/ Felipe Balbi appears to agree with my perspective. http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html does argument-by-authority work? > >> Looking at it from a different perspective, according to the patch set, >> the limits that wm831x is able to impose are: > >> + 0, >> + 2, >> + 100, >> + 500, >> + 900, >> + 1500, >> + 1800, >> + 550, > >> These are, from the battery charger spec, minimums rather than maximums. >> e.g. a CDP provides at least 1500, and as much as 5000. So it seems >> that the wm831x was designed to be told the minimum guaranteed available. >> But that is circumstantial evidence and might be misleading. > > AIUI this is conservatisim in the system design - another way of reading > a spec with a range like this is that the consumer should aim for the > lower limit, the provider should aim for the upper limit and that way if > either of them has issues with tolerances things will still work out. > It predates wide availability of CDP so I wouldn't be surprised if that > bit were just an oversight. Like I say this bit of hardware is totally > separate to the battery charger. Your last sentence is interesting I would reply "of course". All the code we are talking about is only tangentially related to battery charging. It is about how much current can safely be pulled from a USB port. What that current is used for is a completely separate question. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Mon, Sep 12 2016, Mark Brown wrote: > [ Unknown signature status ] > On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote: >> On Mon, Sep 12 2016, Mark Brown wrote: > >> > It's no worse than any other board file situation - if someone has that >> > problem they get to fix it. > >> My point is that the present design does not appear to scale beyond a >> single USB power supply (as if there were two, they would be named in >> discovery order, which is not reliably stable). > > For the practical purposes of people making systems (as opposed to > upstream where this is likely to get most use) it pretty much is. > Though quite how many systems have multiple chargers is itself also a > question. > >> Your point is, I think, that when someone actually cares about that lack >> of scaling, they can fix it. > > Yes. > >> I am perfectly happy with that approach. However if the code doesn't >> scale beyond one charger, it shouldn't pretend that it does. >> i.e. >> Don't have "usb_charger_find_by_name()", just a global "usb_charger" >> (or similar). >> The first charger to register gets to be the "usb_charger". The >> second one gets an error. >> I could be quite happy with that sort of interface. > > Well, a fairly standard way of extending would be to allow the explicit > assignment of names to chargers so this'd avoid such churn. Sure, that might work. I'm just against a design that obviously cannot work. > >> > The whole point from the point of view of the wm831x driver is that it >> > just wants something to tell it how much current it's allowed to draw, I >> > appreciate that doesn't change your analysis of the bit in the middle >> > but the consumer driver bit seems fine here. > >> Yes, the wm831x driver probably does the right thing. >> Other drivers might want to know not only the minimum they are allowed >> to draw, but also the maximum they should try even if they are carefully >> monitoring the voltage. >> So wm831x is doing the right thing with the wrong interface. Maybe you >> can describe that as "fine". > > That's not actually 100% clear to me - for what the wm831x is doing it > probably *does* want the higher limit. This is a system inflow limit > (as it should be for this), at least the charger will adapt to voltage > variations though other users in the system are much less likely to do > so. Interesting ... I hadn't considered that possibility. As long as the current remains below the maximum, the charger will reduce the voltage towards 2V as load increases. Somewhere before it gets there, the system will not be able to make use of the power as the voltage will be too low to be usable. So that will naturally limit the current being drawn. Not having very much electrical engineering background, I cannot say for sure what will happen, but it seems likely that once the voltage drops much below 4.75V, the charger won't be operating at peak efficiency, which would be a waste. I can easily imagine that the hardware would switch off at some voltage level, rather than just making do with what is there. So I'm skeptical of this approach, but I'm open to being corrected by someone more knowledgeable than I. Looking at it from a different perspective, according to the patch set, the limits that wm831x is able to impose are: + 0, + 2, + 100, + 500, + 900, + 1500, + 1800, + 550, These are, from the battery charger spec, minimums rather than maximums. e.g. a CDP provides at least 1500, and as much as 5000. So it seems that the wm831x was designed to be told the minimum guaranteed available. But that is circumstantial evidence and might be misleading. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Mon, Sep 12 2016, Mark Brown wrote: > [ Unknown signature status ] > On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote: >> On Fri, Sep 09 2016, Mark Brown wrote: > >> > The wm831x driver in the patch series is an example of such hardware - >> > it is purely a power manager, it has no USB PHY hardware at all. It's a > >> The "probe" routine calls > >> + usb_charger_find_by_name(wm831x_pdata->usb_gadget); > >> Presumably wm831x_pdata is initialised by a board file? > > Yes. > >> I strongly suspect it is initialized to "usb-charger.0" because the >> names given to usb chargers are "usb-charger.%d" in discovery order. >> I don't see this being at all useful if there is ever more than one >> usb-charger. >> Do you? > > It's no worse than any other board file situation - if someone has that > problem they get to fix it. My point is that the present design does not appear to scale beyond a single USB power supply (as if there were two, they would be named in discovery order, which is not reliably stable). Your point is, I think, that when someone actually cares about that lack of scaling, they can fix it. I am perfectly happy with that approach. However if the code doesn't scale beyond one charger, it shouldn't pretend that it does. i.e. Don't have "usb_charger_find_by_name()", just a global "usb_charger" (or similar). The first charger to register gets to be the "usb_charger". The second one gets an error. I could be quite happy with that sort of interface. > >> So how can this wm831x driver actually find out what sort of charger is >> connected and so set the power limit? I just don't see this working *at* >> *all*. > > The whole point from the point of view of the wm831x driver is that it > just wants something to tell it how much current it's allowed to draw, I > appreciate that doesn't change your analysis of the bit in the middle > but the consumer driver bit seems fine here. Yes, the wm831x driver probably does the right thing. Other drivers might want to know not only the minimum they are allowed to draw, but also the maximum they should try even if they are carefully monitoring the voltage. So wm831x is doing the right thing with the wrong interface. Maybe you can describe that as "fine". NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Fri, Sep 09 2016, Mark Brown wrote:
> [ Unknown signature status ]
> On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:
>
>> Conceptually, the PHY is separate from the power manager and a solution
>> which recognises that will be more universal.
>
> The wm831x driver in the patch series is an example of such hardware -
> it is purely a power manager, it has no USB PHY hardware at all. It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).
It might be instructive to look at exactly what happens with this
device.
The "probe" routine calls
+ usb_charger_find_by_name(wm831x_pdata->usb_gadget);
Presumably wm831x_pdata is initialised by a board file?
I strongly suspect it is initialized to "usb-charger.0" because the
names given to usb chargers are "usb-charger.%d" in discovery order.
I don't see this being at all useful if there is ever more than one
usb-charger.
Do you?
The probe function then registers for charger notifications. When they
arrive, wm831x_usb_limit_change() will set the highest supported limit
which is less than the notified "limit". So presumably that "limit"
must be the minimum guaranteed by the charger type.
Let's see when the notification is called...
->uchgr_nh is sent a notification from usb_charger_notify_others()
with (in the "charger is present" case) the value of
__usb_charger_get_cur_limit(uchger)
which just pulls values out of the cur_limit structure, based on the
type, reported by
usb_charger_get_type_by_others(uchger);
(The default values in this structure are not the minimums guaranteed
by the charger types, they are generally higher. So this could easily
result in the charger shutting down).
usb_charger_get_type_by_others() has two ways to get the charger
type, which it then caches in uchger->type until the charger is removed.
If there is a uchger->psy power supply, then the
POWER_SUPPLY_PROP_CHARGE_TYPE
property is used... Oh, that is weird.
The valid values for that property are:
enum {
POWER_SUPPLY_CHARGE_TYPE_UNKNOWN = 0,
POWER_SUPPLY_CHARGE_TYPE_NONE,
POWER_SUPPLY_CHARGE_TYPE_TRICKLE,
POWER_SUPPLY_CHARGE_TYPE_FAST,
};
but the code in usb_charger_get_type_by_others() compares it against:
+ case POWER_SUPPLY_TYPE_USB:
+ case POWER_SUPPLY_TYPE_USB_DCP:
+ case POWER_SUPPLY_TYPE_USB_CDP:
+ case POWER_SUPPLY_TYPE_USB_ACA:
Presumably that it just a bug and it was meant to request the
POWER_SUPPLY_PROP_TYPE ??
I wonder how much testing was done on this code?
Anyway, assuming it is meant to request the TYPE, where is that set?
The new code doesn't set it at all.
Only three existing power supplies set any of
POWER_SUPPLY_TYPE_USB_*
axp288_charger.c gpio-charger.c isp1704_charger.c
As I wrote in https://lwn.net/Articles/694062/
axp288_charger.c is broken and cannot possibly work.
gpio-charger.c configures the type at boot-time so it cannot sensibly
detect a newly plugged in charger (how does that make any sense)
and isp1704_charger.c peeks in the usb registers (ULPI) to work out
the charger type.
None of these set the POWER_SUPPLY_PROP_TYPE in a useful way, so why
would usb_charger_get_type_by_others() want to use that property?
Maybe it really does want to use POWER_SUPPLY_PROP_CHARGE_TYPE?
Quite a few chargers set that. It would be a challenge to map names
like "TRICKLE" and "FAST" into mA values for a current limiter though.
My hardware knowledge is running out here.. I see wm8350_power.c reports
that property, but I don't know how that device fits into the picture.
With the patch, that driver would request that property from somewhere
else(?) and also report it. That is kind-a strange.
The other possible source for the charger type is a call to
uchger->get_charger_type()
There is no get_charger_type() function anywhere in the patchset. No code
ever sets that field in the uchger.
So how can this wm831x driver actually find out what sort of charger is
connected and so set the power limit? I just don't see this working *at*
*all*.
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Fri, Sep 09 2016, Baolin Wang wrote: >> >> In practice, the USB PHY and the Power manager will often be in the same >> IC (the PMIC) so the driver for one could look at the registers for the >> other. >> But there is no guarantee that the hardware works like that. It is >> best to create a generally design. > > Yes, we hope to create one generally design, so we need to consider > this situation: the power supply getting the charger type by accessing > PMIC registers. The registers which save the charger type are not > always belong to the USB PHY, may be just some registers on PMIC. If the power_supply can directly detect the type of charger, then it doesn't need any usb-charger-infrastructure to tell it. It can handle current selection entirely internally. Surely the only interesting case for a framework to address is the one where the power_supply cannot directly detect the charger type. > > Now in mainline kernel, there are 3 methods can get the charger type > which need to integrate with USB charger framework: > 1. power supply > 2. extcon (need to add as you suggested) > 3. others (by 'get_charger_type' callback of USB charger) There is no "get_charger_type" now in the mainline kernel. >>> >>> Suppose the USB configuration requests 100mA, then we should set the >>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() >>> funtion, then notify this to power driver. >> >> ahh I had missed something there. It's a while since I looked >> closely at these patches. >> >> Only this usage of usb_charger_set_cur_limit_by_type() is really >> nonsensical. >> >> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then >> the number negotiated with the USB configuration is not relevant and >> should be ignored. There is a guaranteed minimum which is at least the >> maximum that *can* be negotiated. > > Yes. If it is not relevant, we will no't set the current from USB > configuration. Just when your charger type is SDP and the USB > enumeration is done, we can get the USB configuration from host to set > current. But you do! The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is. So when it is not relevant, your code *does* set some current limit. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Thu, Sep 08 2016, Baolin Wang wrote: > On 8 September 2016 at 15:31, NeilBrown wrote: >> On Thu, Sep 08 2016, Baolin Wang wrote: >>> >>> Now the usb charger will not get charger type from 'extcon' subsystem, >>> we get the charger type from 'power_supply' and calllback >>> 'get_charger_type' for users. >> >> I understand this. I think it is wrong because, in general, the >> power_supply doesn't know what the charger_type is (it might know it is >> USB, but most don't know which sort of USB). The PHY knows that, not >> the power_supply. > > I don't think so. Now many platforms will detect the charger type by > PMIC hardware, and we can get the charger type by PMIC hardware > register. Then power supply driver can access the PMIC register to get > the charger type. Here USB charger just considers if the accessing the > PMIC register to get charger type is implemented in power supply, it > is optional depending on what your platform designed. > In practice, the USB PHY and the Power manager will often be in the same IC (the PMIC) so the driver for one could look at the registers for the other. But there is no guarantee that the hardware works like that. It is best to create a generally design. Conceptually, the PHY is separate from the power manager and a solution which recognises that will be more universal. If the power manager can always just look at that phy registers to know what sort of charger is connected, why does you framework need to work with charger types at all? >>> >>> Yes, but you must think about some special cases on some platforms. >>> Users may need to change the current in some situations, thus we >>> should export one API for users to change the current. (I think you >>> misunderstand the current limit here, that is the current for power >>> driver to draw). >> >> Can you be specific about these "special cases" please? >> I cannot think of any. > > Suppose the USB configuration requests 100mA, then we should set the > USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() > funtion, then notify this to power driver. ahh I had missed something there. It's a while since I looked closely at these patches. Only this usage of usb_charger_set_cur_limit_by_type() is really nonsensical. If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then the number negotiated with the USB configuration is not relevant and should be ignored. There is a guaranteed minimum which is at least the maximum that *can* be negotiated. It is only when the cable appears to be a SDP (standard downstream port) that the usb-config negotiation is relevant. That is because the minimum guaranteed for SDP is only 100mA. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Thu, Sep 08 2016, Baolin Wang wrote: > Hi Neil, > > On 6 September 2016 at 13:40, NeilBrown wrote: >> On Mon, Aug 29 2016, Baolin Wang wrote: >> >>> Hi Felipe, >>> >>> On 11 August 2016 at 11:14, Baolin Wang wrote: >>>> Hi Felipe, >>>> >>>> On 1 August 2016 at 15:09, Baolin Wang wrote: >>>>> Currently the Linux kernel does not provide any standard integration of >>>>> this >>>>> feature that integrates the USB subsystem with the system power regulation >>>>> provided by PMICs meaning that either vendors must add this in their >>>>> kernels >>>>> or USB gadget devices based on Linux (such as mobile phones) may not >>>>> behave >>>>> as they should. Thus provide a standard framework for doing this in >>>>> kernel. >>>>> >>>>> Now introduce one user with wm831x_power to support and test the usb >>>>> charger, >>>>> which is pending testing. Moreover there may be other potential users >>>>> will use >>>>> it in future. >>>> >>>> Could you please apply this patchset into your 'next' branch if you >>>> have no comments about it? Thank you. >>> >>> Since there are no other comments about this patchset for a long time, >>> could you please apply this patchset? Thanks. >> > > Sorry for the late reply. > >> Sorry, I should have replied earlier. Tim Bird mentioned on the >> ksummit-discuss list that there was a frustration with this not making >> progress so I decided to contribute what I could now. >> >> I think this patch set is attempting to address an important problem >> that needs solving. However I think it gets some key aspects wrong. >> Maybe they can get fixed up after the patchset is upstream, maybe they >> should be fixed first - I have no strong opinion on that. >> >> My main complaints involve the detection and handling of the different >> charger types - DCP, CDP, ACA etc. >> The big-picture requirement here that the PHY will detect the physical >> properties of the cable (e.g. resistance to ground on ID) and determine >> the type of charger expected. This information must be communicated to >> the PMIC "power_supply" device so it can regulate the power being drawn >> through the cable. >> >> The first problem is that there are two different ways that the >> distinction between DCP, CDP, ACA etc can be represented in Linux. They >> are cable types in the 'extcon' subsystem, and they are power_supply >> types in the 'power_supply' subsystem. This duplication is confusing. >> It is not caused by your patch set, but I believe your patchset needs to >> work with the duplication and I think it does so poorly. > > Now the usb charger will not get charger type from 'extcon' subsystem, > we get the charger type from 'power_supply' and calllback > 'get_charger_type' for users. I understand this. I think it is wrong because, in general, the power_supply doesn't know what the charger_type is (it might know it is USB, but most don't know which sort of USB). The PHY knows that, not the power_supply. > >> >> In my mind, the power_supply should *not* know about this distinction at >> all (except possibly as an advisor attribute simiarly to the current >> battery technology attribute). The other types it knows of are "AC", >> "USB", and "BATTERY". The contrast between these is quite different >> From the contrast between DCP, CDP, ACA, which, from the perspective of >> the power supply, are almost irrelevant. Your patchset effectively >> examines the power_supply_type of one power_supply, and communicates it >> to another. It isn't clear to me how the first power_supply gets the >> information, or what the relationship between the two power_supplies is >> meant to be. > > We just get the charger type from the power supply which can get the > charger type from register or something else, But that register would be part of the PHY, not part of the charger. Having the power_supply driver reading the PHY register might work for some hardware, but is not a general solution. >and the usb charger did > nothing for this power supply. In some platform, the charger type is > get in power supply driver, thus we should link this power supply to > get the charger type when USB cable is plugin. If you don't get > ch
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
support for this in the code, but it is not universally acknowledged. For a USB charging framework to be genuinely useful, it must (I think) make sure this issue gets clarified, and the various cable types used properly. Once the cable/charger type has been determined, the PMIC power_supply needs to use this information. Your current patches make use of this information in ways that are wrong in two respects. Firstly, you have made the current limit associated with each cable type configurable (__usb_charger_set_cur_limit_by_type). This is nonsense. The standard (e.g. BC-1.2) declares what the current limits are. There is no reason for those not to be hard coded. Secondly, you treat each charger type as having a single "cur_limit" and utilize that limit by telling the PMIC to draw that much current. Again, this is inconsistent with the specification. BC-1.2 defines, for each charger type, a minimum and maximum current level. The minimum should always be available. Attempting to draw more than that (but less that the max) might work or might cause the charger to shut down, but you can be sure that the charger will respond to the increased load by first reducing the voltage, and will not shut down until the voltage has dropped below 2V. If you try to draw more current than the maximum, then the charger might shut down before the voltage drops below 2V. Given this understanding of the current available from the charger, there are two approaches the PMIC might take. 1/ if the PMIC is able to exercise fine control over the current it draws, and if it can monitor the voltage on the charger, then it could gradually increase the power being requested until the voltage drops below some threshold (e.g. 4.75V), and then (probably) back off a little. It should only increase at most up to the maximum, even if the voltage remains high. It should probably start at zero rather than at the minimum. This allows for lossage elsewhere. 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine control of the current requested, then it should request only the minimum available current. Any more is not safe. So your USB charging framework should communicate the MIN and MAX as specified in the standard, and allow the PMIC to use that information as appropriate. A library routine to support the incremental increase in current drain would probably be appreciated by PMIC driver writers. A more details discussion of this problem-space can be found at https://lwn.net/Articles/693027/ and an analysis of the functionality currently available in the kernel (which adds a number of specifics to the above) can be found at https://lwn.net/Articles/694062/ NeilBrown signature.asc Description: PGP signature
Re: [PATCH] hso: fix refcnt leak in recent patch.
On Tue, 14 Apr 2015 08:50:01 +0200 Olivier Sobrie wrote:
> Hello Neil,
>
> On Tue, Apr 14, 2015 at 11:03:03AM +1000, NeilBrown wrote:
> > On Tue, 14 Apr 2015 09:36:34 +1000 NeilBrown wrote:
> >
> > >
> > >
> > > Prior to
> > > commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> > > hso: fix crash when device disappears while serial port is open
> > >
> > > hso_serial_open would always kref_get(&serial->parent->ref) before
> > > returning zero.
> > > Since that commit, it only calls kref_get when returning 0 if
> > > serial->port.count was zero.
> > >
> > > This results in calls to
> > >kref_put(&serial->parent->ref, hso_serial_ref_free);
> > >
> > > after hso_serial_ref_free has been called, which dereferences a freed
> > > pointer.
> > >
> > > This patch adds the missing kref_get().
> > >
> > > Fixes: commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> > > Cc: sta...@vger.kernel.org (v4.0)
> > > Cc: Olivier Sobrie
> > > Signed-off-by: NeilBrown
> > >
> > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > > index 75befc1bd816..6848fc903340 100644
> > > --- a/drivers/net/usb/hso.c
> > > +++ b/drivers/net/usb/hso.c
> > > @@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty,
> > > struct file *filp)
> > > }
> > > } else {
> > > D1("Port was already open");
> > > + kref_get(&serial->parent->ref);
> > > }
> > >
> > > usb_autopm_put_interface(serial->parent->interface);
> >
> >
> > Sorry - that was wrong.
> > I'm getting crashes which strongly suggest the kref_put is being called
> > extra
> > times, but I misunderstood the code and was hasty.
> >
> > Maybe this instead?
>
> Indeed, if I undestand correctly the code in tty_io.c, cleanup() method
> is also called when the open fails while kref_get is only done if the
> open succeeds. Sorry for that mess.
> I assume you get that crash when hso_start_serial_device() returns
> an error?
Yes, that's correct.
>
> At first sight, the patch below looks good to me.
> I'll test it in the next days.
Thanks.
NeilBrown
>
> Thank you,
>
> Olivier
>
> >
> > Thanks,
> > NeilBrown
> >
> > From: NeilBrown
> > Date: Tue, 14 Apr 2015 09:33:03 +1000
> > Subject: [PATCH] hso: fix refcnt leak in recent patch.
> >
> > Prior to
> > commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> > hso: fix crash when device disappears while serial port is open
> >
> > a kref_get on serial->parent->ref would be taken on each open,
> > and it would be kref_put on each close.
> >
> > Now the kref_put happens when the tty_struct is finally put (via
> > the 'cleanup') providing tty->driver_data has been set.
> > So the kref_get must be called exact once when tty->driver_data is
> > set.
> >
> > With the current code, if the first open fails the kref_get() is never
> > called, but the kref_put() is called, leaving to a crash.
> >
> > So change the kref_get call to happen exactly when ->driver_data is
> > changed from NULL to non-NULL.
> >
> > Fixes: commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> > Cc: sta...@vger.kernel.org (v4.0)
> > Cc: Olivier Sobrie
> > Signed-off-by: NeilBrown
> >
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 75befc1bd816..17fd3820263a 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -1278,6 +1278,8 @@ static int hso_serial_open(struct tty_struct *tty,
> > struct file *filp)
> > D1("Opening %d", serial->minor);
> >
> > /* setup */
> > + if (tty->driver_data == NULL)
> > + kref_get(&serial->parent->ref);
> > tty->driver_data = serial;
> > tty_port_tty_set(&serial->port, tty);
> >
> > @@ -1294,8 +1296,6 @@ static int hso_serial_open(struct tty_struct *tty,
> > struct file *filp)
> > if (result) {
> > hso_stop_serial_device(serial->parent);
> > serial->port.count--;
> > - } else {
> > - kref_get(&serial->parent->ref);
> > }
> > } else {
> > D1("Port was already open");
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
pgpYnt5TgA7PH.pgp
Description: OpenPGP digital signature
Re: [PATCH] hso: fix refcnt leak in recent patch.
On Tue, 14 Apr 2015 09:36:34 +1000 NeilBrown wrote:
>
>
> Prior to
> commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> hso: fix crash when device disappears while serial port is open
>
> hso_serial_open would always kref_get(&serial->parent->ref) before
> returning zero.
> Since that commit, it only calls kref_get when returning 0 if
> serial->port.count was zero.
>
> This results in calls to
>kref_put(&serial->parent->ref, hso_serial_ref_free);
>
> after hso_serial_ref_free has been called, which dereferences a freed
> pointer.
>
> This patch adds the missing kref_get().
>
> Fixes: commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
> Cc: sta...@vger.kernel.org (v4.0)
> Cc: Olivier Sobrie
> Signed-off-by: NeilBrown
>
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 75befc1bd816..6848fc903340 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty,
> struct file *filp)
> }
> } else {
> D1("Port was already open");
> + kref_get(&serial->parent->ref);
> }
>
> usb_autopm_put_interface(serial->parent->interface);
Sorry - that was wrong.
I'm getting crashes which strongly suggest the kref_put is being called extra
times, but I misunderstood the code and was hasty.
Maybe this instead?
Thanks,
NeilBrown
From: NeilBrown
Date: Tue, 14 Apr 2015 09:33:03 +1000
Subject: [PATCH] hso: fix refcnt leak in recent patch.
Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open
a kref_get on serial->parent->ref would be taken on each open,
and it would be kref_put on each close.
Now the kref_put happens when the tty_struct is finally put (via
the 'cleanup') providing tty->driver_data has been set.
So the kref_get must be called exact once when tty->driver_data is
set.
With the current code, if the first open fails the kref_get() is never
called, but the kref_put() is called, leaving to a crash.
So change the kref_get call to happen exactly when ->driver_data is
changed from NULL to non-NULL.
Fixes: commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
Cc: sta...@vger.kernel.org (v4.0)
Cc: Olivier Sobrie
Signed-off-by: NeilBrown
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 75befc1bd816..17fd3820263a 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1278,6 +1278,8 @@ static int hso_serial_open(struct tty_struct *tty, struct
file *filp)
D1("Opening %d", serial->minor);
/* setup */
+ if (tty->driver_data == NULL)
+ kref_get(&serial->parent->ref);
tty->driver_data = serial;
tty_port_tty_set(&serial->port, tty);
@@ -1294,8 +1296,6 @@ static int hso_serial_open(struct tty_struct *tty, struct
file *filp)
if (result) {
hso_stop_serial_device(serial->parent);
serial->port.count--;
- } else {
- kref_get(&serial->parent->ref);
}
} else {
D1("Port was already open");
pgpvV8Sbj5jsm.pgp
Description: OpenPGP digital signature
[PATCH] hso: fix refcnt leak in recent patch.
Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open
hso_serial_open would always kref_get(&serial->parent->ref) before
returning zero.
Since that commit, it only calls kref_get when returning 0 if
serial->port.count was zero.
This results in calls to
kref_put(&serial->parent->ref, hso_serial_ref_free);
after hso_serial_ref_free has been called, which dereferences a freed
pointer.
This patch adds the missing kref_get().
Fixes: commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
Cc: sta...@vger.kernel.org (v4.0)
Cc: Olivier Sobrie
Signed-off-by: NeilBrown
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 75befc1bd816..6848fc903340 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty, struct
file *filp)
}
} else {
D1("Port was already open");
+ kref_get(&serial->parent->ref);
}
usb_autopm_put_interface(serial->parent->interface);
pgpJLoeJoEfMC.pgp
Description: OpenPGP digital signature
[PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
From: NeilBrown
twl4030_charger currently finds the associated phy
using usb_get_phy() which will return the first USB2 phy.
If your platform has multiple such phys (as mine does),
this is not reliable (and reliably fails on the GTA04).
Change to use devm_usb_get_phy_by_node(), having found the
node by looking for an appropriately named sibling in
device-tree.
This makes usb-charging dependent on correct device-tree
configuration.
Acked-by: Pavel Machek
Signed-off-by: NeilBrown
---
.../devicetree/bindings/power/twl-charger.txt | 10 ++
.../devicetree/bindings/usb/twl-usb.txt|3 +++
drivers/power/twl4030_charger.c| 21 +---
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/power/twl-charger.txt
b/Documentation/devicetree/bindings/power/twl-charger.txt
index d5c706216df5..3b4ea1b73b38 100644
--- a/Documentation/devicetree/bindings/power/twl-charger.txt
+++ b/Documentation/devicetree/bindings/power/twl-charger.txt
@@ -1,5 +1,15 @@
TWL BCI (Battery Charger Interface)
+The battery charger needs to interact with the USB phy in order
+to know when charging is permissible, and when there is a connection
+or disconnection.
+
+The choice of phy cannot be configured at a hardware level, so there
+is no value in explicit configuration in device-tree. Rather
+if there is a sibling of the BCI node which is compatible with
+"ti,twl4030-usb", then that is used to determine when and how
+use USB power for charging.
+
Required properties:
- compatible:
- "ti,twl4030-bci"
diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
b/Documentation/devicetree/bindings/usb/twl-usb.txt
index 0aee0ad3f035..17327a296110 100644
--- a/Documentation/devicetree/bindings/usb/twl-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -30,6 +30,9 @@ TWL4030 USB PHY AND COMPARATOR
- usb_mode : The mode used by the phy to connect to the controller. "1"
specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
+If a sibling node is compatible "ti,twl4030-bci", then it will find
+this device and query it for USB power status.
+
twl4030-usb {
compatible = "ti,twl4030-usb";
interrupts = < 10 4 >;
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index d35b83e635b5..b07f4e2f2dde 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -629,10 +629,15 @@ static int __init twl4030_bci_probe(struct
platform_device *pdev)
INIT_WORK(&bci->work, twl4030_bci_usb_work);
- bci->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
- usb_register_notifier(bci->transceiver, &bci->usb_nb);
+ bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
+ if (bci->dev->of_node) {
+ struct device_node *phynode;
+
+ phynode = of_find_compatible_node(bci->dev->of_node->parent,
+ NULL, "ti,twl4030-usb");
+ if (phynode)
+ bci->transceiver = devm_usb_get_phy_by_node(
+ bci->dev, phynode, &bci->usb_nb);
}
/* Enable interrupts now. */
@@ -662,10 +667,6 @@ static int __init twl4030_bci_probe(struct platform_device
*pdev)
return 0;
fail_unmask_interrupts:
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
- usb_put_phy(bci->transceiver);
- }
free_irq(bci->irq_bci, bci);
fail_bci_irq:
free_irq(bci->irq_chg, bci);
@@ -694,10 +695,6 @@ static int __exit twl4030_bci_remove(struct
platform_device *pdev)
twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
TWL4030_INTERRUPTS_BCIIMR2A);
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
- usb_put_phy(bci->transceiver);
- }
free_irq(bci->irq_bci, bci);
free_irq(bci->irq_chg, bci);
power_supply_unregister(&bci->usb);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Allow twl4030_charger to find phy reliably.
The twl4030_charger is physically paired with the twl4030 USB phy, so the drivers need to be able to reliably find each other. twl4030_charger currently uses usb_get_phy(), which works if there is only one phy to choose from, but is not reliable in more complex configurations. These patches add a new interface to allow a phy to be found given a device node, and then use that interface in twl4030_charger so that it finds its sibling in the devicetree, and gets the phy associated with that. === This is a resend with improved documentation. I'm hoping this can go in to the next merge window, though should it go through usb/phy or power??? Thanks, NeilBrown --- NeilBrown (2): usb: phy: Add interface to get phy give of device_node. twl4030_charger: find associated phy by more reliable means. .../devicetree/bindings/power/twl-charger.txt | 10 ++ .../devicetree/bindings/usb/twl-usb.txt|3 + drivers/power/twl4030_charger.c| 21 ++-- drivers/usb/phy/phy.c | 97 ++-- include/linux/usb/phy.h|2 5 files changed, 94 insertions(+), 39 deletions(-) -- Signature -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: phy: Add interface to get phy give of device_node.
From: NeilBrown
Split the "get phy from device_node" functionality out of
"get phy by phandle" so it can be used directly.
This is useful when a battery-charger is intimately associated with a
particular phy but handled by a separate driver. The charger
can find the device_node based on sibling relationships
without the need for a redundant declaration in the devicetree
description.
As a peripheral that gets a phy will often want to register a
notifier block, and de-register it later, that functionality
is included so the de-registration is automatic.
Acked-by: Pavel Machek
Signed-off-by: NeilBrown
---
drivers/usb/phy/phy.c | 97 ++-
include/linux/usb/phy.h |2 +
2 files changed, 72 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 2f9735b35338..6b089e19e8db 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -22,6 +22,11 @@ static LIST_HEAD(phy_list);
static LIST_HEAD(phy_bind_list);
static DEFINE_SPINLOCK(phy_lock);
+struct phy_devm {
+ struct usb_phy *phy;
+ struct notifier_block *nb;
+};
+
static struct usb_phy *__usb_find_phy(struct list_head *list,
enum usb_phy_type type)
{
@@ -79,6 +84,15 @@ static void devm_usb_phy_release(struct device *dev, void
*res)
usb_put_phy(phy);
}
+static void devm_usb_phy_release2(struct device *dev, void *_res)
+{
+ struct phy_devm *res = _res;
+
+ if (res->nb)
+ usb_unregister_notifier(res->phy, res->nb);
+ usb_put_phy(res->phy);
+}
+
static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
{
return res == match_data;
@@ -151,40 +165,30 @@ err0:
EXPORT_SYMBOL_GPL(usb_get_phy);
/**
- * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * devm_usb_get_phy_by_node - find the USB PHY by device_node
* @dev - device that requests this phy
- * @phandle - name of the property holding the phy phandle value
- * @index - the index of the phy
+ * @node - the device_node for the phy device.
+ * @nb - a notifier_block to register with the phy.
*
- * Returns the phy driver associated with the given phandle value,
+ * Returns the phy driver associated with the given device_node,
* after getting a refcount to it, -ENODEV if there is no such phy or
- * -EPROBE_DEFER if there is a phandle to the phy, but the device is
- * not yet loaded. While at that, it also associates the device with
+ * -EPROBE_DEFER if the device is not yet loaded. While at that, it
+ * also associates the device with
* the phy using devres. On driver detach, release function is invoked
* on the devres data, then, devres data is freed.
*
- * For use by USB host and peripheral drivers.
+ * For use by peripheral drivers for devices related to a phy,
+ * such as a charger.
*/
-struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
- const char *phandle, u8 index)
+struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
+ struct device_node *node,
+ struct notifier_block *nb)
{
- struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
+ struct usb_phy *phy = ERR_PTR(-ENOMEM);
+ struct phy_devm *ptr;
unsigned long flags;
- struct device_node *node;
- if (!dev->of_node) {
- dev_dbg(dev, "device does not have a device node entry\n");
- return ERR_PTR(-EINVAL);
- }
-
- node = of_parse_phandle(dev->of_node, phandle, index);
- if (!node) {
- dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
- dev->of_node->full_name);
- return ERR_PTR(-ENODEV);
- }
-
- ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
+ ptr = devres_alloc(devm_usb_phy_release2, sizeof(*ptr), GFP_KERNEL);
if (!ptr) {
dev_dbg(dev, "failed to allocate memory for devres\n");
goto err0;
@@ -203,8 +207,10 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device
*dev,
devres_free(ptr);
goto err1;
}
-
- *ptr = phy;
+ if (nb)
+ usb_register_notifier(phy, nb);
+ ptr->phy = phy;
+ ptr->nb = nb;
devres_add(dev, ptr);
get_device(phy->dev);
@@ -213,10 +219,47 @@ err1:
spin_unlock_irqrestore(&phy_lock, flags);
err0:
- of_node_put(node);
return phy;
}
+EXPORT_SYMBOL_GPL(devm_usb_get_phy_by_node);
+
+/**
+ * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * @dev - device that requests this phy
+ * @phandle - name of the property holding the phy phandle value
+ * @index - the index of the phy
+ *
+ * Returns the phy driver associated with the given phandle value,
+ * after getting a refcou
Re: [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
On Wed, 25 Feb 2015 22:13:51 +0100 Sebastian Reichel wrote:
> Hi Neil,
>
> On Tue, Feb 24, 2015 at 03:01:29PM +1100, NeilBrown wrote:
> > twl4030_charger currently finds the associated phy
> > using usb_get_phy() which will return the first USB2 phy.
> > If your platform has multiple such phys (as mine does),
> > this is not reliable (and reliably fails on the GTA04).
> >
> > Change to use devm_usb_get_phy_by_node(), having found the
> > node by looking for an appropriately named sibling in
> > device-tree.
> >
> > This makes usb-charging dependent on correct device-tree
> > configuration.
>
> The patch looks ok to me, but you should update the DT documentation
> in Documentation/devicetree/bindings/power/twl-charger.txt regarding
> the sibling dependency.
>
> Apart from that DT binding maintainers should be CC'd.
>
> -- Sebastian
Thanks.
I've added the following. I've also changed the code to use
of_find_compatible_node() and find the USB phy based on 'compatible' rather
than on the node name.
I'll Cc DT maintainers when I resubmit.
Thanks,
NeilBrown
diff --git a/Documentation/devicetree/bindings/power/twl-charger.txt
b/Documentation/devicetree/bindings/power/twl-charger.txt
index d5c706216df5..3b4ea1b73b38 100644
--- a/Documentation/devicetree/bindings/power/twl-charger.txt
+++ b/Documentation/devicetree/bindings/power/twl-charger.txt
@@ -1,5 +1,15 @@
TWL BCI (Battery Charger Interface)
+The battery charger needs to interact with the USB phy in order
+to know when charging is permissible, and when there is a connection
+or disconnection.
+
+The choice of phy cannot be configured at a hardware level, so there
+is no value in explicit configuration in device-tree. Rather
+if there is a sibling of the BCI node which is compatible with
+"ti,twl4030-usb", then that is used to determine when and how
+use USB power for charging.
+
Required properties:
- compatible:
- "ti,twl4030-bci"
diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
b/Documentation/devicetree/bindings/usb/twl-usb.txt
index 0aee0ad3f035..17327a296110 100644
--- a/Documentation/devicetree/bindings/usb/twl-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -30,6 +30,9 @@ TWL4030 USB PHY AND COMPARATOR
- usb_mode : The mode used by the phy to connect to the controller. "1"
specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
+If a sibling node is compatible "ti,twl4030-bci", then it will find
+this device and query it for USB power status.
+
twl4030-usb {
compatible = "ti,twl4030-usb";
interrupts = < 10 4 >;
pgpqpsfi56l6F.pgp
Description: OpenPGP digital signature
[PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
twl4030_charger currently finds the associated phy
using usb_get_phy() which will return the first USB2 phy.
If your platform has multiple such phys (as mine does),
this is not reliable (and reliably fails on the GTA04).
Change to use devm_usb_get_phy_by_node(), having found the
node by looking for an appropriately named sibling in
device-tree.
This makes usb-charging dependent on correct device-tree
configuration.
Signed-off-by: NeilBrown
---
drivers/power/twl4030_charger.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index d35b83e635b5..4cf5ffbc904a 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -629,10 +629,15 @@ static int __init twl4030_bci_probe(struct
platform_device *pdev)
INIT_WORK(&bci->work, twl4030_bci_usb_work);
- bci->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
- usb_register_notifier(bci->transceiver, &bci->usb_nb);
+ bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
+ if (bci->dev->of_node) {
+ struct device_node *phynode;
+
+ phynode = of_get_child_by_name(bci->dev->of_node->parent,
+ "twl4030-usb");
+ if (phynode)
+ bci->transceiver = devm_usb_get_phy_by_node(
+ bci->dev, phynode, &bci->usb_nb);
}
/* Enable interrupts now. */
@@ -662,10 +667,6 @@ static int __init twl4030_bci_probe(struct platform_device
*pdev)
return 0;
fail_unmask_interrupts:
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
- usb_put_phy(bci->transceiver);
- }
free_irq(bci->irq_bci, bci);
fail_bci_irq:
free_irq(bci->irq_chg, bci);
@@ -694,10 +695,6 @@ static int __exit twl4030_bci_remove(struct
platform_device *pdev)
twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
TWL4030_INTERRUPTS_BCIIMR2A);
- if (!IS_ERR_OR_NULL(bci->transceiver)) {
- usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
- usb_put_phy(bci->transceiver);
- }
free_irq(bci->irq_bci, bci);
free_irq(bci->irq_chg, bci);
power_supply_unregister(&bci->usb);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Allow twl4030_charger to find phy reliably.
The twl4030_charger is physically paired with the twl4030 USB phy, so the drivers need to be able to reliably find each other. twl4030_charger currently uses usb_get_phy(), which works if there is only one phy to choose from, but is not reliable in more complex configurations. These patches add a new interface to allow a phy to be found given a device node, and then use that interface in twl4030_charger so that it finds its sibling in the devicetree, and gets the phy associated with that. Thanks, NeilBrown --- NeilBrown (2): usb: phy: Add interface to get phy give of device_node. twl4030_charger: find associated phy by more reliable means. drivers/power/twl4030_charger.c | 21 drivers/usb/phy/phy.c | 97 --- include/linux/usb/phy.h |2 + 3 files changed, 81 insertions(+), 39 deletions(-) -- Signature -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: phy: Add interface to get phy give of device_node.
Split the "get phy from device_node" functionality out of
"get phy by phandle" so it can be used directly.
This is useful when a battery-charger is intimately associated with a
particular phy but handled by a separate driver. The charger
can find the device_node based on sibling relationships
without the need for a redundant declaration in the devicetree
description.
As a peripheral that gets a phy will often want to register a
notifier block, and de-register it later, that functionality
is included so the de-registration is automatic.
Signed-off-by: NeilBrown
---
drivers/usb/phy/phy.c | 97 ++-
include/linux/usb/phy.h |2 +
2 files changed, 72 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 2f9735b35338..6b089e19e8db 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -22,6 +22,11 @@ static LIST_HEAD(phy_list);
static LIST_HEAD(phy_bind_list);
static DEFINE_SPINLOCK(phy_lock);
+struct phy_devm {
+ struct usb_phy *phy;
+ struct notifier_block *nb;
+};
+
static struct usb_phy *__usb_find_phy(struct list_head *list,
enum usb_phy_type type)
{
@@ -79,6 +84,15 @@ static void devm_usb_phy_release(struct device *dev, void
*res)
usb_put_phy(phy);
}
+static void devm_usb_phy_release2(struct device *dev, void *_res)
+{
+ struct phy_devm *res = _res;
+
+ if (res->nb)
+ usb_unregister_notifier(res->phy, res->nb);
+ usb_put_phy(res->phy);
+}
+
static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
{
return res == match_data;
@@ -151,40 +165,30 @@ err0:
EXPORT_SYMBOL_GPL(usb_get_phy);
/**
- * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * devm_usb_get_phy_by_node - find the USB PHY by device_node
* @dev - device that requests this phy
- * @phandle - name of the property holding the phy phandle value
- * @index - the index of the phy
+ * @node - the device_node for the phy device.
+ * @nb - a notifier_block to register with the phy.
*
- * Returns the phy driver associated with the given phandle value,
+ * Returns the phy driver associated with the given device_node,
* after getting a refcount to it, -ENODEV if there is no such phy or
- * -EPROBE_DEFER if there is a phandle to the phy, but the device is
- * not yet loaded. While at that, it also associates the device with
+ * -EPROBE_DEFER if the device is not yet loaded. While at that, it
+ * also associates the device with
* the phy using devres. On driver detach, release function is invoked
* on the devres data, then, devres data is freed.
*
- * For use by USB host and peripheral drivers.
+ * For use by peripheral drivers for devices related to a phy,
+ * such as a charger.
*/
-struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
- const char *phandle, u8 index)
+struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
+ struct device_node *node,
+ struct notifier_block *nb)
{
- struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
+ struct usb_phy *phy = ERR_PTR(-ENOMEM);
+ struct phy_devm *ptr;
unsigned long flags;
- struct device_node *node;
- if (!dev->of_node) {
- dev_dbg(dev, "device does not have a device node entry\n");
- return ERR_PTR(-EINVAL);
- }
-
- node = of_parse_phandle(dev->of_node, phandle, index);
- if (!node) {
- dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
- dev->of_node->full_name);
- return ERR_PTR(-ENODEV);
- }
-
- ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
+ ptr = devres_alloc(devm_usb_phy_release2, sizeof(*ptr), GFP_KERNEL);
if (!ptr) {
dev_dbg(dev, "failed to allocate memory for devres\n");
goto err0;
@@ -203,8 +207,10 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device
*dev,
devres_free(ptr);
goto err1;
}
-
- *ptr = phy;
+ if (nb)
+ usb_register_notifier(phy, nb);
+ ptr->phy = phy;
+ ptr->nb = nb;
devres_add(dev, ptr);
get_device(phy->dev);
@@ -213,10 +219,47 @@ err1:
spin_unlock_irqrestore(&phy_lock, flags);
err0:
- of_node_put(node);
return phy;
}
+EXPORT_SYMBOL_GPL(devm_usb_get_phy_by_node);
+
+/**
+ * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * @dev - device that requests this phy
+ * @phandle - name of the property holding the phy phandle value
+ * @index - the index of the phy
+ *
+ * Returns the phy driver associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such p
Re: [PATCH] usb: musb: fix context save over suspend.
On Tue, 12 Feb 2013 13:03:36 -0800 Kevin Hilman wrote: > NeilBrown writes: > > > On Mon, 21 Jan 2013 13:38:59 +0200 Igor Grinberg > > wrote: > > > >> -BEGIN PGP SIGNED MESSAGE- > >> Hash: SHA1 > >> > >> Hi Neil, > >> > >> On 01/21/13 11:28, NeilBrown wrote: > >> > > >> > > >> > The standard suspend sequence involves runtime_resuming > >> > devices before suspending the system. > >> > So just saving context in runtime_suspend and restoring it > >> > in runtime resume isn't enough. We must also save in "suspend" > >> > and restore in "resume". > >> > > >> > Without this patch, and OMAP3 system with off_mode enabled will find > >> > the musb port non-functional after suspend/resume. With the patch it > >> > works perfectly. > >> > >> Hmmm... Some time ago, this has been removed in > >> 5d193ce8 (usb: musb: PM: fix context save/restore in suspend/resume path) > >> > >> Am I missing something? Or things changed and now this patch is correct? > > > > Hi Igor, > > thanks for alerting me to that patch does anyone else get the feeling > > that power management to too complex to be understood by a mere human? > > Yes. ;) > > > That commit (5d193ce8) suggests that the musb-hdrc device is an > > 'omap_device', or maybe has a PM domain set to something else. > > However it isn't/doesn't. dev->pm_domain is NULL. So no PM domain layer > > will ever call the musb_core musb_runtime_suspend/resume. > > > > The parent device - musb-omap2430 - is an omap device, does have pm_domain > > set, and does have its omap2430_runtime_suspend/resume called for system > > suspend and so the context for that device is saved and restored. > > However that doesn't help the context for musb-hdrc. > > > > Whether musb ever was an omap_device is beyond my archaeological skills to > > determine. > > > > Kevin: Was musb-hdrc ever a device with a pm_domain? or was it only ever > > the various possible parents that had domains? > > Are you able to defend your earlier patch in today's kernel? It > > certainly causes my device not to work properly. > > Sorry for the delay here, I'm back to a place where I can test this on > real hardware. > > My patch was fixing a real hang when musb was built-in (or loaded), in > host-mode (mini-A cable attached) but no devices attached. I just tried > to reproduce this, and with your patch, the system hangs during suspend. > Odd. I plug in a mini-A cable, note that the 'mode' file holds 'a_idle' (sometimes just plugging in the cable isn't enough to trigger that, but sometimes it is) and suspend/resume work perfectly. Though after resume it is back to b_idle. unplug/replug and it is back to a_idle. suspend/resume and back to b_idle. > That being said, your description makes sense why this context > save/restore is needed. Perhaps your patch needs to add a check whether > the device is runtime suspended (I gather this is what Ruslan's patch is > doing.) I'm not sure it is possible for the device to be runtime suspended at this point. Certainly my device never is, even if it was just before the suspend sequence started. Something is waking it up... (instruments the code). Ahh - usb_suspend() calls choose_wakeup() which might call pm_runtime_resume() if the could be a need to reprogram the wakeup setting. As that is a 'might', the device might not be runtime-awake when 'suspend' runs. Can you see if this, on top of my previous patch, does any better on your hardware? Thanks, NeilBrown diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index 956db0e..00deb94 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2278,7 +2278,8 @@ static int musb_suspend(struct device *dev) } spin_unlock_irqrestore(&musb->lock, flags); - musb_save_context(musb); + if (!pm_runtime_status_suspended(dev)) + musb_save_context(musb); return 0; } @@ -2289,7 +2290,8 @@ static int musb_resume_noirq(struct device *dev) * module got reset through the PSC (vs just being disabled). */ struct musb *musb = dev_to_musb(dev); - musb_restore_context(musb); + if (!pm_runtime_status_suspended(dev)) + musb_restore_context(musb); return 0; } signature.asc Description: PGP signature
Re: [PATCH] usb: musb: fix context save over suspend.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On Mon, 21 Jan 2013 13:38:59 +0200 Igor Grinberg
wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> Hi Neil,
>
> On 01/21/13 11:28, NeilBrown wrote:
> >
> >
> > The standard suspend sequence involves runtime_resuming
> > devices before suspending the system.
> > So just saving context in runtime_suspend and restoring it
> > in runtime resume isn't enough. We must also save in "suspend"
> > and restore in "resume".
> >
> > Without this patch, and OMAP3 system with off_mode enabled will find
> > the musb port non-functional after suspend/resume. With the patch it
> > works perfectly.
>
> Hmmm... Some time ago, this has been removed in
> 5d193ce8 (usb: musb: PM: fix context save/restore in suspend/resume path)
>
> Am I missing something? Or things changed and now this patch is correct?
Hi Igor,
thanks for alerting me to that patch does anyone else get the feeling
that power management to too complex to be understood by a mere human?
That commit (5d193ce8) suggests that the musb-hdrc device is an
'omap_device', or maybe has a PM domain set to something else.
However it isn't/doesn't. dev->pm_domain is NULL. So no PM domain layer
will ever call the musb_core musb_runtime_suspend/resume.
The parent device - musb-omap2430 - is an omap device, does have pm_domain
set, and does have its omap2430_runtime_suspend/resume called for system
suspend and so the context for that device is saved and restored.
However that doesn't help the context for musb-hdrc.
Whether musb ever was an omap_device is beyond my archaeological skills to
determine.
Kevin: Was musb-hdrc ever a device with a pm_domain? or was it only ever
the various possible parents that had domains?
Are you able to defend your earlier patch in today's kernel? It
certainly causes my device not to work properly.
Thanks,
NeilBrown
>
> >
> > Signed-off-by: NeilBrown
> >
> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > index fd34867..b6ccc02 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -2225,6 +2225,7 @@ static int musb_suspend(struct device *dev)
> > }
> >
> > spin_unlock_irqrestore(&musb->lock, flags);
> > + musb_save_context(musb);
> > return 0;
> > }
> >
> > @@ -2234,6 +2235,8 @@ static int musb_resume_noirq(struct device *dev)
> > * unless for some reason the whole soc powered down or the USB
> > * module got reset through the PSC (vs just being disabled).
> > */
> > + struct musb *musb = dev_to_musb(dev);
> > + musb_restore_context(musb);
> > return 0;
> > }
> >
>
> - --
> Regards,
> Igor.
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.17 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQIcBAEBAgAGBQJQ/SjTAAoJEBDE8YO64EfaHrsP/2bl4rP6L/tWLSZ+rNEdz6B+
> Qo+HVOhnTVsOxgWbbd5VrfhE28jLoFGMslrLuI+geeCcJ1zgwNsahG9C11bygyfu
> 54hQgkmaxDJPDKAlalcy7VK9C6tOTgQV5iSbuRlemttK879dTrb+33zP6idn5+zK
> kxptY38fpmyojnl8gJiVa6Plik/apQcVr+GIx8CMwj+YQC5vkdg7cUEWyngfyk2C
> W0U4NceroS8NSjRbcFV3V6Q912TVjKzl+B2yxVD0OBaSK4BpHEncDBXiVx8APq87
> 4nDeBB5gDXi1rtN3YjcfDaFu0me5qzpYc3JFFidvdLTdXIdvxDzjHgMqsZB8ZBYC
> R0e5PtIw/62I90d63JkXZXVRTB7JeZsGfZFY2R7MxBab9or8zz0OyYwGWoW63vzH
> oFrwmAkWoD0IEKcfc8+dd99eicgZrmQL6FDWrEMsX+RS34LRtVU30SVAudRhY+CR
> MhNCjKyFySwx7wqkGgJl1ECl0Y6U4ua0v4bv7kdE6eyrgbQIkiGliSJ7DhWBPcP6
> iMIOTwC7+LwPYP/MX2uYR3DXDfI0XwiqdtyzhD9LJe4PRol8zjozS2j0Y7FriItw
> jFqsgCgwDc9j8ufcpXf5ZynJYnlCG0iLuAPEUugZot83/CpxgU++A8cuHqUrOnhH
> 76L95rflUTkpiQ76ffP7
> =jqXb
> -END PGP SIGNATURE-
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (GNU/Linux)
iQIVAwUBUP21WDnsnt1WYoG5AQIQlQ/+LlXi1ZRXKtO/oj/u4iqs7DL8PNcZX6C7
j7Rrx8nxd2C15pEnxvOYIrojLTrXqz3bgE9NmBvYOYY+dj2/+CaS7RJsTR0gYJi0
AMkLT9k6+edUJ79KtIRmeqbnPEnujOWHkBdg2dLrm4PpYmGUmc8VGeE3+mh3La97
ssZm4gYbip6TPzB4MdvTbhkxbEXJidOcgBJrsdTvMXB8HkZ30Wq6/YELtYoYNpZo
rn+CbQo5Dd0bUb8fQ3Fd5unfXqx5N5txBslwK8JgSA3L7l96d3+q9UOfBg/PsUJJ
WrnFahv11lypajHtnCnXb3z+TsGgV4v46aUZ4Yk+tkwVv81nbORHTRGoaLReRMG2
Sii/xYeugOuhnJI//07yWrnLfunFbJJyHmfZARz/6kKNoIPrZwRDmVeO3+Iu/39R
zmwvJDTqONwenXnZEmxqrON0E/Y8V6hdNlGZdFYIypJr/Ym2rp+R+qcRyEwQxAYi
7h1mACvXE7tYCCoBi+fN5hFaF2VQeN1QqJMTimQjqnkgaI2jQ4Zm7zMCUKREhGPu
3TTvOwuFGM+bwb2eKsW+4zSzebdepXaNjSPCmHSKU++5SVcOMNiZyKlrvoW8znM4
/1Ea+3CmkHqAhmja5Fly4NYL2fdy/NhfIqZI2yIrTAG58iIankQjBHysqAcGrvQp
TplHj7osO40=
=G91I
-END PGP SIGNATURE-
N§²æìr¸yúèØb²X¬¶Ç§vØ^)Þº{.nÇ+·¥{±ºÆâØ^nr¡ö¦zËëh¨èÚ&¢ø ®G«éh®(éÝ¢j"ú¶m§ÿïêäz¹Þàþf£¢·h§~m
[PATCH] usb: musb: fix context save over suspend.
The standard suspend sequence involves runtime_resuming devices before suspending the system. So just saving context in runtime_suspend and restoring it in runtime resume isn't enough. We must also save in "suspend" and restore in "resume". Without this patch, and OMAP3 system with off_mode enabled will find the musb port non-functional after suspend/resume. With the patch it works perfectly. Signed-off-by: NeilBrown diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index fd34867..b6ccc02 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2225,6 +2225,7 @@ static int musb_suspend(struct device *dev) } spin_unlock_irqrestore(&musb->lock, flags); + musb_save_context(musb); return 0; } @@ -2234,6 +2235,8 @@ static int musb_resume_noirq(struct device *dev) * unless for some reason the whole soc powered down or the USB * module got reset through the PSC (vs just being disabled). */ + struct musb *musb = dev_to_musb(dev); + musb_restore_context(musb); return 0; } signature.asc Description: PGP signature
Re: Infinite looping in omap2430.c USB driver
On Mon, 13 Aug 2012 17:32:34 +0300 Felipe Balbi wrote:
> Hi,
>
> On Mon, Aug 13, 2012 at 12:34:53PM +1000, NeilBrown wrote:
> > On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi wrote:
> >
> >
> > > hehe, that's nasty. Please send a patch converting to a try count and a
> > > udelay_range(), or something.
> > >
> >
> > how's this?
> >
> > Thanks,
> > NeilBrown
> >
> >
> > From: NeilBrown
> > Date: Mon, 13 Aug 2012 12:32:58 +1000
> > Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.
> >
> > When called during resume_irqs, omap2430_musb_set_vbus() is run with
> > interrupts disabled, In that case 'jiffies' never changes so the loop
> > can loop forever.
> >
> > So impose a maximum loop count and add an 'mdelay' to ensure we wait
> > a reasonable amount of time for bit to be cleared.
> >
> > This fixes a hang on resume.
> >
> > Signed-of-by: NeilBrown
> >
> > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> > index c7785e8..8a93381 100644
> > --- a/drivers/usb/musb/omap2430.c
> > +++ b/drivers/usb/musb/omap2430.c
> > @@ -34,6 +34,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "musb_core.h"
> > #include "omap2430.h"
> > @@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb,
> > int is_on)
> >
> > if (is_on) {
> > if (musb->xceiv->state == OTG_STATE_A_IDLE) {
> > + int loops = 100;
> > /* start the session */
> > devctl |= MUSB_DEVCTL_SESSION;
> > musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > @@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb,
> > int is_on)
> > */
> > while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> >
> > + mdelay(5);
>
> I would prefer udelay_range() as it will let scheduler group timers.
> Something like:
>
> udelay_range(3000, 5000);
>
> should do, I gues...
>
Except that there is no udelay_range :-(
There is a usleep_range, but that can only be used from non-atomic context
and in the problem case interrupts are disabled and a spinlock is held so we
very definitely are not in non-atomic context. If we need a delay at all, it
has to be udelay or mdelay.
If we could do this in a work function rather than directly from the
interrupt handler that would be best but I have no idea what dependencies
there are.. Would it be safe for musb_stage0_irq() to ask a workqueue to run
musb_platform_set_vbus rather than doing it directly?
NeilBrown
signature.asc
Description: PGP signature
Re: Infinite looping in omap2430.c USB driver
On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi wrote:
> hehe, that's nasty. Please send a patch converting to a try count and a
> udelay_range(), or something.
>
how's this?
Thanks,
NeilBrown
From: NeilBrown
Date: Mon, 13 Aug 2012 12:32:58 +1000
Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.
When called during resume_irqs, omap2430_musb_set_vbus() is run with
interrupts disabled, In that case 'jiffies' never changes so the loop
can loop forever.
So impose a maximum loop count and add an 'mdelay' to ensure we wait
a reasonable amount of time for bit to be cleared.
This fixes a hang on resume.
Signed-of-by: NeilBrown
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index c7785e8..8a93381 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include "musb_core.h"
#include "omap2430.h"
@@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb, int
is_on)
if (is_on) {
if (musb->xceiv->state == OTG_STATE_A_IDLE) {
+ int loops = 100;
/* start the session */
devctl |= MUSB_DEVCTL_SESSION;
musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
@@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb, int
is_on)
*/
while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
+ mdelay(5);
cpu_relax();
- if (time_after(jiffies, timeout)) {
+ if (time_after(jiffies, timeout)
+ || loops-- <= 0) {
dev_err(musb->controller,
"configured as A device timeout");
ret = -EINVAL;
signature.asc
Description: PGP signature
Re: Infinite looping in omap2430.c USB driver
Hi Felipe,
have you had a chance to look at this problem in omap2430_mbus_set_vbus yet?
Are you the person responsible?
thanks,
NeilBrown
On Mon, 9 Jul 2012 01:32:33 -0700 Tony Lindgren wrote:
> * NeilBrown [120706 15:44]:
> >
> > Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`
> >
> > omap2430_musb_set_vbus in omap2430.c contains:
> >
> > while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> >
> > cpu_relax();
> >
> > if (time_after(jiffies, timeout)) {
> > dev_err(musb->controller,
> > "configured as A device timeout");
> > ret = -EINVAL;
> > break;
> > }
> > }
> >
> > having set
> > unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> >
> > so it can busy-loop for up to 1 second. Probably not ideal, but if it works
> > I wouldn't complain.
> >
> > The
> > if (int_usb & MUSB_INTR_SESSREQ) {
> > branch of musb_stage0_irq() called from musb_interrupt (from
> > generic_interrupt) calls this:
> >
> > if (musb->int_usb)
> > retval |= musb_stage0_irq(musb, musb->int_usb,
> > devctl, power);
> >
> > so the busy loop can happen in an interrupt handler (not a threaded
> > interrupt
> > handler), which is probably less ideal.
> >
> > However this can be called with interrupt disabled, as happens at least
> > during resume when resume_irqs() calls:
> >
> > raw_spin_lock_irqsave(&desc->lock, flags);
> > __enable_irq(desc, irq, true);
> > raw_spin_unlock_irqrestore(&desc->lock, flags);
> >
> > and an interrupt is found to be IRQS_PENDING.
> >
> > In this case interrupts are disabled so 'jiffies' never changes so this loop
> > can continue forever.
> >
> > This happens on my (GTA04) phone fairly regularly - between 1 in 10 and 1 in
> > 30 resumes. The musb-hdrc interrupt is pending and reports
> >
> > [ 4957.624176] musb-hdrc musb-hdrc: ** IRQ peripheral usb0040 tx rx
> >
> > 'usb0040' is MUSB_INTR_SESSREQ. I think this is triggered by detecting a
> > voltage change on the USB ID pin - is that right? A short-to-earth would be
> > a request to switch to host mode, which is why it tries to enable VBUS.
> > Maybe there is some electrical noise which is being picked up?
>
> I guess that could happen if the transceiver pins are floating during suspend?
>
> > In any case I get the interrupt despite nothing being plugged in, and the
> > 0x80
> > bit of MUSB_DEVCTL never gets cleared.
>
> As far as I remember, musb tries to be smart about changing to host mode,
> and tries to do the session and vbus detection on it's own.. AFAIK, there's
> nothing you can do until musb is done and detects the VBUS is not rising and
> gives up. There are all kind of interrupt flag combinations trying to deal
> with that mess, maybe you need to add yet another one?
>
> > I've added a simple loop counter which aborts the loop after 1000 loops -
> > this takes about 5 seconds, but includes some printks which probably slow it
> > down.
> >
> > In 2 out of 2 cases, subsequent messages show that the hsmmc driver for the
> > uSD card that holds my root filesystem is messed up. It seems to be waiting
> > for a request that is never going to complete.
> > So maybe the hsmmc is causing the noise that triggers the musb issue.
> >
> > I can send a patch which add a loop count if you like, but I suspect you can
> > come up with a much better approach.
>
> Sounds like that loop should be fixed.
>
> Regards,
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
signature.asc
Description: PGP signature
Infinite looping in omap2430.c USB driver
Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`
omap2430_musb_set_vbus in omap2430.c contains:
while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
cpu_relax();
if (time_after(jiffies, timeout)) {
dev_err(musb->controller,
"configured as A device timeout");
ret = -EINVAL;
break;
}
}
having set
unsigned long timeout = jiffies + msecs_to_jiffies(1000);
so it can busy-loop for up to 1 second. Probably not ideal, but if it works
I wouldn't complain.
The
if (int_usb & MUSB_INTR_SESSREQ) {
branch of musb_stage0_irq() called from musb_interrupt (from
generic_interrupt) calls this:
if (musb->int_usb)
retval |= musb_stage0_irq(musb, musb->int_usb,
devctl, power);
so the busy loop can happen in an interrupt handler (not a threaded interrupt
handler), which is probably less ideal.
However this can be called with interrupt disabled, as happens at least
during resume when resume_irqs() calls:
raw_spin_lock_irqsave(&desc->lock, flags);
__enable_irq(desc, irq, true);
raw_spin_unlock_irqrestore(&desc->lock, flags);
and an interrupt is found to be IRQS_PENDING.
In this case interrupts are disabled so 'jiffies' never changes so this loop
can continue forever.
This happens on my (GTA04) phone fairly regularly - between 1 in 10 and 1 in
30 resumes. The musb-hdrc interrupt is pending and reports
[ 4957.624176] musb-hdrc musb-hdrc: ** IRQ peripheral usb0040 tx rx
'usb0040' is MUSB_INTR_SESSREQ. I think this is triggered by detecting a
voltage change on the USB ID pin - is that right? A short-to-earth would be
a request to switch to host mode, which is why it tries to enable VBUS.
Maybe there is some electrical noise which is being picked up?
In any case I get the interrupt despite nothing being plugged in, and the 0x80
bit of MUSB_DEVCTL never gets cleared.
I've added a simple loop counter which aborts the loop after 1000 loops -
this takes about 5 seconds, but includes some printks which probably slow it
down.
In 2 out of 2 cases, subsequent messages show that the hsmmc driver for the
uSD card that holds my root filesystem is messed up. It seems to be waiting
for a request that is never going to complete.
So maybe the hsmmc is causing the noise that triggers the musb issue.
I can send a patch which add a loop count if you like, but I suspect you can
come up with a much better approach.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
